Skip to content

Conversation

@johnhunter
Copy link
Contributor

Closes #

What I did

Implements changes discussed in #23909 and reverts changes from PR #23709

  1. Remove Volta config from root package.json
  2. Add an .nvmrc file to the project root
  3. Update contributing docs to suggest using FNM for local development.
  4. Update GH actions to use the .nvmrc rather than hardcoded node versions.

Open issues for review

  • The CircleCI workflows use cimgs with specific node versions. Configuring those to use the .nvmrc file would be involved. It might be possible with Dynamic configs although maybe not worth it.
  • RFC feedback suggests not using the .nvmrc node version in the publish script. It looks like these would be called in the canary-release and publish actions which where already hardcoded to node@16

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@johnhunter
Copy link
Contributor Author

Danger task failing on branch due to a 403 on the GitHub user "Resource not accessible by integration".

"Could not add a commit status, the GitHub token for Danger does not have access rights."

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me! 👍

Don't worry about CircleCI, it's not worth it right now. One day we might migrate to dynamic configs for other reasons.

I'll defer to @kylegach for the docs changes.

@JReinhold JReinhold changed the title Manage Node version from .nvmrc and revert Volta config Development: Manage Node version with .nvmrc Aug 30, 2023
@JReinhold JReinhold added build Internal-facing build tooling & test updates ci:normal labels Aug 30, 2023
Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @johnhunter. I have no issue with these docs changes.

I'm fairly sure most contributors use nvm, though...

@johnhunter
Copy link
Contributor Author

Thanks @kylegach

I'm fairly sure most contributors use nvm, though...

I think that's true, for them the changes will just work and for new contributors fnm is a faster tool.

I'm not sure of the process - @JReinhold are you able to merge this PR?

@johnhunter johnhunter force-pushed the node-version-nvmrc branch 2 times, most recently from 961cb8f to 65329ea Compare September 4, 2023 08:31
@johnhunter
Copy link
Contributor Author

Force-pushed from merge conflicts with upstream next

Add .nvmrc config for latest node 16
Publish and release related but these actions already used hardcoded node 16 versions.
@yannbf yannbf merged commit f48ad32 into storybookjs:next Sep 4, 2023
@github-actions github-actions bot mentioned this pull request Sep 4, 2023
25 tasks
@johnhunter johnhunter deleted the node-version-nvmrc branch September 4, 2023 16:47
shilman pushed a commit that referenced this pull request Sep 6, 2023
Development: Manage Node version with .nvmrc
@lawyoum
Copy link

lawyoum commented Nov 2, 2023

Release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Internal-facing build tooling & test updates ci:normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants