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

To what extent is NVM needed for the local environment? #9120

Closed
ktmn opened this issue Aug 18, 2018 · 6 comments
Closed

To what extent is NVM needed for the local environment? #9120

ktmn opened this issue Aug 18, 2018 · 6 comments
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Question Questions about the design or development of the editor.

Comments

@ktmn
Copy link

ktmn commented Aug 18, 2018

I cloned the repo and ran ./bin/setup-local-env.sh, it wants me to install nvm. I use n so installing nvm as well could cause conflicts.

Couldn't the bin/install-node-nvm.sh script just check if I have an up to date node version and just skip all that?

To what extent is the nvm needed? If I comment out the nvm part is that going to cause problems?

@designsimply designsimply added [Type] Question Questions about the design or development of the editor. [Type] Build Tooling Issues or PRs related to build tooling labels Aug 18, 2018
@kwight
Copy link

kwight commented Aug 29, 2018

I hit this too (having been happily using n for quite a while).

To get you up and running:

  • n lts will get you the Node version the script is looking for.
  • Open bin/setup-local-env.sh, and replace the . "$(dirname "$0")/install-node-nvm.sh" line with npm install (that's the one piece from that script you really need).
  • Run ./bin/setup-local-env.sh, and you will get the expected startup process.

I tend to agree, having nvm so deeply engrained isn't the best experience – the script may require adding a new dotfile, and will quietly upgrade npm, two side effects which may not be appreciated by everyone. Granted, if a developer is starting from scratch, it's pretty darn convenient.

Two options forward would be:

  • Work checks using the n binary into the existing script. It'll get kinda messy quickly though :/
  • Simplify the script to just do the checks, and leave nvm (or n!) installation up to the user.

I'd probably lean to the latter.

@ntwb
Copy link
Member

ntwb commented Aug 30, 2018

I've had similar issues, though in my case I use the Fish Shell and nvm is tricky with fish, so much so that I actually use a custom function named nvm to have nvm functionality, though its not 100% compatible and one of the conflicts I have is basically the same as above, nvm is not detected by the ./bin/setup-local-env.sh script and therefore fails.

I'm not sure on the ideal solution either, to be honest, it would be great to explore some of the options already listed above though

@kwight
Copy link

kwight commented Mar 9, 2019

Noting that as of now, the above hack no longer works, and everything hangs in package installation. I eventually gave up, removed n, and went with nvm 😞 .

@gziolo
Copy link
Member

gziolo commented Apr 23, 2019

I don't think that nvm is necessary at all. It's mostly there to ensure that Travis runs with the latest version of node and npm. It should be perfectly fine to add logic to the ./bin/setup-local-env.sh which detects n and uses it instead as well. I'm closing this issue. If you think that we should add detection of n, feel free to open another issue.

Related code:

# Check Node and NVM are installed
. "$(dirname "$0")/install-node-nvm.sh"

https://github.com/WordPress/gutenberg/blob/e21255689c5299bb865efdea5ca1d0543a379d60/bin/install-node-nvm.sh

@kwight
Copy link

kwight commented Aug 31, 2019

None of this should be an issue anymore with the merge of #17004 :

Much like the WordPress environment, however, I've aimed to not automate everything. As we found with the Gutenberg environment, too much automation tended to make it difficult (particularly for more advanced contributors) to customise or debug their environment. For example, the scripts no longer install NVM, switch your Node version, or run npm install.

🎉

@pento
Copy link
Member

pento commented Sep 2, 2019

The .nvmrc file is still there, so you can use NVM if you prefer it. It's no longer a requirement for the automated environment scripts, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

No branches or pull requests

6 participants