CLI: Check node and npm version

[fluiddot]

The script failed with the following error when creating a release:

Found '/var/folders/s7/lwl9q12j2_31hw8y1vkmln180000gn/T/gbm-2543640981/.nvmrc' with version <20>
Now using node v20.10.0 (npm v10.2.5)
npm ERR! code EBADENGINE
npm ERR! engine Unsupported engine
npm ERR! engine Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Required: {"node":">=20.10.0","npm":">=10.2.3"}
npm ERR! notsup Actual:   {"npm":"8.19.4","node":"v16.20.2"}

I didn't have the default node version set to the expected version of Gutenberg, which explains the error. However, it failed after creating the branches. I propose checking for the node and npm versions needed to run the scripts in Gutenberg and Gutenberg Mobile as a first step.

Ideally, the script should switch to the needed version by running nvm use.

[dcalhoun]

Definitely understand any confusion from the error and experience.

Ideally, the script should switch to the needed version by running nvm use.

If possible, I would guide us against making nvm a hard dependency. Transparently, that guidance is self-serving as I rely upon asdf-vm, but it could be good to avoid dependencies nonetheless. A few alternatives might be:

• Documenting that individuals should set up deeper shell integration for nvm on development machines.
• Adding a check that runs at the beginning of the gbm-cli script to verify the global Node.js version is as expected if nvm is installed.
• Make nvm a dependency but provide a --no-nvm flag to skip its commands.

[fluiddot]

If possible, I would guide us against making nvm a hard dependency. Transparently, that guidance is self-serving as I rely upon asdf-vm, but it could be good to avoid dependencies nonetheless.

I completely agree. I'd also advocate making the tool agnostic to the node version manager. Another alternative we could follow is providing an argument to switch the node version. E.g --switch-node-command "nvm use"

[jhnstn]

However, it failed after creating the branches.

Just locally right? All it should do before using npm is checkout the repo and switch branches. I just want to make sure it didn't push anything before failing.
The issue with verifying node before checking out is that we need the .nvmrc file to know which node to use.

I suppose we could fetch the .nvmrc directly from Github before cloning the repo. I'm not sure if that is worth the effort however. I think we just need to update the docs on what to do when npm fails.

Ideally, the script should switch to the needed version by running nvm use

It should be doing that but looks the logic is wrong. It's looking to see if the script is running on CI. I believe I thought the SetupNode util would handle local runs that but looking now I can see how that doesn't help. I opened this issue to fix that: #240

But from the error log it looks like the script was able to set up node correctly, so doesn't look like there are any problems with your nvm setup:

Found '/var/folders/s7/lwl9q12j2_31hw8y1vkmln180000gn/T/gbm-2543640981/.nvmrc' with version <20>
Now using node v20.10.0 (npm v10.2.5)

That is misleading since the next npm commands were not prefixed with loading nvm

The issue is that Go executes commands in a nested bash shell. It's possible to set a different shell in the command but that could get complicated.

One work around is to add the nvm set up to ~/.bash_profile. I just tried that with node@16 as my default and it correctly switched to node@20

If possible, I would guide us against making nvm a hard dependency.

It's not a hard dependency.
We considered accounting for other node managers but opted to fallback to using the globally installed node. I do think we should make sure it works with nvm since it's still the recommended node manager for the Gutenberg project

Make nvm a dependency but provide a --no-nvm flag to skip its commands.

This already happens by checking for theNVM_DIR env so there is no need to add another flag.
There is a possibility that someone does want to use nvm but has some bespoke setup. In that case it would still fall back to the global installed node. Also another case for providing documentation on diagnosing what could be broken e.g. Enter a nested shell bash -l and run echo $NVM_DIR to see if nvm is reachable.

I'd also advocate making the tool agnostic to the node version manager. Another alternative we could follow is providing an argument to switch the node version. E.g --switch-node-command "nvm use"

That could get complicated very quickly. As noted above, Go runs commands in bash so if the supplied switch command is only set up in the users default shell then it won't work. We'd have to add another flag to specify the shell or a script that will load the preferred node manager into bash. Or have folks make sure the node manager is loaded in ~/.bash_profile

Since this is a highly specialized tool used by a few people, I think the best option is to fix #240 and provide guidance on how to wire in other node managers as needed.

[dcalhoun]

If possible, I would guide us against making nvm a hard dependency.

It's not a hard dependency. We considered accounting for other node managers but opted to fallback to using the globally installed node.

Correct. I was specifically providing feedback on the suggestion to invoke nvm use and that we should avoid changing the current relationship with nvm, if feasible. I was not stating there was an existing hard dependency. As you shared, the script is already conditionally invoking nvm commands.

I do think we should make sure it works with nvm since it's still the recommended node manager for the Gutenberg project

I agree the CLI should work with nvm by default for the reason you stated.

Also another case for providing documentation on diagnosing what could be broken e.g. Enter a nested shell bash -l and run echo $NVM_DIR to see if nvm is reachable.

Great idea!

That could get complicated very quickly. As noted above, Go runs commands in bash so if the supplied switch command is only set up in the users default shell then it won't work.

That does get complex quickly. To be clear, this is the type of complexity threshold where I think it would be very reasonable to reject support for nvm alternatives. I am not against doing so. I am merely advocating to continue avoiding the hard dependency for as long as reasonably possible.

Thank you for your hard work on this CLI. It is an meaningfully impactful tool for lowering the project's operational overheard.

[jhnstn]

I was specifically providing feedback on the suggestion to invoke nvm use and that we should avoid changing the current relationship with nvm, if feasible

Ah ok. makes sense. I don't foresee any reason to make nvm a hard dependency.

That does get complex quickly. To be clear, this is the type of complexity threshold where I think it would be very reasonable to reject support for nvm alternatives. I am not against doing so. I am merely advocating to continue avoiding the hard dependency for as long as reasonably possible.

I think we are an the same page here. As I mentioned, since this is such a specialized tool for a small group of people I think it's fine to wire in a few different node managers. To be honest I can only think of 3: nvm ,n and asdf. I believe our team only uses nvm and asdf. I'm pretty sure we could hook into asdf in a graceful way but just not dynamically by supplying a command at run time.

I don't think there is a risk of falling back to a hard dependency. I do remember throwing that at you while you were on leave with the Jetpack setup in Mobile Gutenberg 😄 . Genuinely don't want to do that again !