Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add volta configuration for managing node versions #19705

Closed
wants to merge 4 commits into from

Conversation

creativecoder
Copy link
Contributor

Changes proposed in this Pull Request:

This adds a volta entries for all package.json files, pinning the node version for those using Volta.

Some of the benefits over nvm include

  • No need to run nvm use to select the correct node version.
  • When the node version is updated, it's installed automatically.
  • Globally installed commands (e.g. yarn) are pinned correctly to the project version, without the need for using npx.
  • Avoids issues that nvm has with alternative shell environments, such as running npm/yarn commands through VS Code's shell.

Volta can replace nvm, but those who wish to continuing using nvm can. As long as the project provides an .nvmrc file, nvm use overrides volta, in my testing.

Related wp-calypso PR.

Jetpack product discussion

n/a

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Install volta (brew install volta will do it on macOS with homebrew)
  • In a directory outside the Jetpack repo, add node and yarn to your volta toolchain, e.g.
$ volta install node@latest
success: installed and set node@16.0.0 (with npm@7.10.0) as default
$ volta install yarn
success: installed and set yarn@1.22.10 as default
$ volta list
⚡️ Currently active tools:

    Node: v16.0.0 (default)
    Yarn: v1.22.10 (default)
  • Now switch to the jetpack repo directory with this branch checked out and run volta list, you should see the versions specified in package.json
$ volta list
⚡️ Currently active tools:

    Node: v14.16.0 (current @ .../jetpack/package.json)
    Yarn: v1.22.10 (current @ .../jetpack/package.json)
  • And the same for all of the packages in the repo
$ cd projects/plugins/jetpack 
$ volta list
⚡️ Currently active tools:

    Node: v14.16.0 (current @
    .../jetpack/projects/plugins/jetpack/package.json)
    Yarn: v1.22.10 (current @
    .../jetpack/projects/plugins/jetpack/package.json)

Copy link

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

Connection

  • In-place connection with free plan
  • In-place connection with paid plan
  • In-place connection with product purchase
  • Classic connection. Use Safari, or set a constant JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME to true
  • Disconnect/reconnect connection
  • Secondary user connection
  • Connection on multisite

Verify that the changes are compatible with the plugins that use the connection package.

  • WooCommerce Payments
  • Jetpack Boost
  • Previous versions of Jetpack

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello creativecoder! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D60791-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@creativecoder creativecoder self-assigned this Apr 29, 2021
@creativecoder creativecoder added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Apr 29, 2021
@github-actions github-actions bot added [Package] Connection UI This package no longer exists in the monorepo. [Package] Jitm [Package] Lazy Images This package no longer exists in the monorepo. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Tools] Development CLI The tools/cli to assist during JP development. labels Apr 29, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • 🔴 Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: May 4, 2021.
  • Scheduled code freeze: April 26, 2021.

@github-actions github-actions bot added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 29, 2021
@creativecoder creativecoder added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 29, 2021
@creativecoder creativecoder requested a review from a team April 29, 2021 14:41
@creativecoder creativecoder changed the title Adds volta configuration for managing node versions Add volta configuration for managing node versions Apr 29, 2021
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

That seems like an interesting tool! It's a bit of a pity that volta cannot rely on the existing engines info though, that makes for a lot of duplication. Is there a way around that?

@@ -29,5 +29,8 @@
"engines": {
"node": "^14.16.0",
"yarn": "^1.3.2"
},
"volta": {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we'd want to extend here (and for the other packages), since the JITM package is also available standalone, in a situation where there is nothing to extend:
https://github.com/Automattic/jetpack-jitm/blob/master/package.json

Copy link
Contributor

Choose a reason for hiding this comment

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

If nothing else, we could copy the configuration and use a script (run from precommit and CI) to ensure that the copies remain in sync and are in sync with .nvmrc. We could also verify that the "engines" are in sync everywhere too, and that the versions are compatible.

Or we could wait for volta to resolve volta-cli/volta#282 so we could just drop a .voltarc or whatever in the monorepo root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we'd want to extend here (and for the other packages), since the JITM package is also available standalone...

Makes sense! I'd forgotten about the mirror repos. I've updated the PR so that each package.json has the version info rather than extending the root, except for projects/plugins/jetpack/tests/e2e/package.json, which now extends projects/plugins/jetpack/package.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also verify that the "engines" are in sync everywhere too, and that the versions [in .nvmrc versus package.json "engines.node"] are compatible.

FYI, I pushed #19748 to ensure that engines in package.json is in sync everywhere, and #19750 to check that the version in .nvmrc satisfies that called for by package.json.

Once those are merged, it should be straightforward to do the same for the volta blocks.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 29, 2021
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

That seems like an interesting tool! It's a bit of a pity that volta cannot rely on the existing engines info though, that makes for a lot of duplication. Is there a way around that?

+1. I'm wary of adding a bunch of extra configuration for tools people may not actually use.

@@ -0,0 +1,4 @@
Significance: minor
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems not really "minor". It doesn't change the code at all, it's just configuring a dev tool.

Suggested change
Significance: minor
Significance: patch

Same in your other changelog files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@creativecoder
Copy link
Contributor Author

I'm wary of adding a bunch of extra configuration for tools people may not actually use.

@anomiex That's totally a fair concern. Though I'd bet that this tool will become quite popular at a8c, particularly for WP.com developers, because we tend to interact with so many different product repos on a daily basis. When the node versions they use get out of sync (which seems to be frequently), it tends to cause problems, even with nvm configured properly. You can see one example of these troubles described here: 98-gh-Automattic/a8c-wp-env.

Any one who's run into The engine "node" is incompatible with this module. Expected version... when running an npm/yarn command will probably like using Volta!

@creativecoder
Copy link
Contributor Author

That seems like an interesting tool! It's a bit of a pity that volta cannot rely on the existing engines info though, that makes for a lot of duplication. Is there a way around that?

@jeherve Agreed! I don't see any way around this currently. It looks like there are a few different proposals in the works, e.g. volta-cli/volta#959. Discussions seem to be proceeding slowly, as it appears to me they're prioritizing a long-lasting, stable solution, that accounts for the variety of different ways of specifying node versions.

@creativecoder creativecoder added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 29, 2021
@kraftbj
Copy link
Contributor

kraftbj commented Apr 29, 2021

As a note, I looked into it a bit and volta intentionally doesn't reuse engines, see volta-cli/volta#742 (comment) amongst others.

volta intends to represent the specific version to develop on while engines represent the range that the project will work on. By definition, volta does not support ranges thus separate key.

Looking at 959, it sounds more like a way to easily pin a "supported" version of node vs just grabbing the latest LTS, which isn't ideal.

I know traditionally Jetpack has aimed to update our nvmrc as soon as possible once Calypso does. While that works well-enough probably for us, with any other project added into the mix, I can appreciate the amount of dents must be in some desks from banging heads against them...

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels May 21, 2021
@jeherve
Copy link
Member

jeherve commented Jul 1, 2021

On hold for now.

Internal reference: p7H4VZ-31V-p2

In short, let's aim to upgrade to Node 16 first so everyone gets native Node binaries with Volta (reference)

@sirreal
Copy link
Member

sirreal commented Jul 2, 2021

Volta hasn't implemented pnpm support yet: volta-cli/volta#737

@jeherve
Copy link
Member

jeherve commented Mar 2, 2022

I'll close this PR for now because of the lack of activity on this. We can always reopen in the future if needed, but it will need a rebase, so it may be easier to start a new PR at this point.

@jeherve jeherve closed this Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Connection UI This package no longer exists in the monorepo. [Package] Jitm [Package] Lazy Images This package no longer exists in the monorepo. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Status] Blocked / Hold [Tools] Development CLI The tools/cli to assist during JP development. Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants