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

Change .nvmrc and documentation for Node.js version (LTS to 14.18.1) #36744

Merged
merged 5 commits into from
Nov 23, 2021
Merged

Change .nvmrc and documentation for Node.js version (LTS to 14.18.1) #36744

merged 5 commits into from
Nov 23, 2021

Conversation

alexstine
Copy link
Contributor

@alexstine alexstine commented Nov 22, 2021

Description

Instead of using Node.js lts version in .nvmrc and other documentation, this PR switches items to use version 14.18.1. I've found that the lts version can vary from system to system and this has gotten me a couple times. It seems hardcoding the correct version is the appropriate option to fix any confusion going forward.

Why the Change?

This is what happens when running the nvm install command as mentioned in documentation.

`alexstine@asm gutenberg % nvm install --lts

Installing latest LTS version.

v16.13.0 is already installed.

Now using node v16.13.0 (npm v8.1.0)`

I'm not sure exactly why this started happening, but these versions do not work with Gutenberg.

`alexstine@asm gutenberg % node --version

v16.13.0

alexstine@asm gutenberg % npm --version

8.1.0`

While I am not in favor of using older versions, using what works is a must. After asking @tellthemachines on Slack, I decided that a PR with the correct version instead of a generic "lts/*" may be an improvement.

I kind of wonder if this is just some strange issue with my system and this doesn't effect others. In that case, this can be closed out.

How has this been tested?

Whenever I run nvm use, the correct version is loaded up and things just work. 👍

Screenshots

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@gziolo
Copy link
Member

gziolo commented Nov 22, 2021

There is another PR opened nearly 2 weeks ago which pins node to 14.x line rather than to the specific version which is going to change quickly. See #36384 from @walbo. I think the only blocker for using LTS is the fact that Node 16.x that was promoted to LTS a few weeks ago is using npm v8 rather than npm v6 that we still have to use.

I recently noticed that many projects use --legacy-peer-deps with npm 7/8 as a temporary meassure needed until the whole ecosystem is able to handle automatic peer dependencies installation. Maybe we should follow the same path and migrate quicker to Node 16 + npm 8. The only blocker I see is what @desrosj raised a few weeks back that we should keep the same versions as in WordPress Core to ensure it builds correctly with the infrastructre in place.

@alexstine
Copy link
Contributor Author

@gziolo If I change to v14, will it allow us to get this in sooner? Every time I face issues with this and I have to imagine others do as well. This also updates the all important docs to better reflect the versions.

@gziolo
Copy link
Member

gziolo commented Nov 22, 2021

@gziolo If I change to v14, will it allow us to get this in sooner? Every time I face issues with this and I have to imagine others do as well. This also updates the all important docs to better reflect the versions.

Yes, it's also what @youknowriad did for wp/5.8 branch a few days ago:

https://github.com/WordPress/gutenberg/blob/wp/5.8/.nvmrc

and what we did in the past (version number used changes over time) for branches containing code for older major WordPress releases.

Using LTS sounds great in overall, but the change happens once per year, so maybe it's fine to apply the change explicitly with a PR like this one.

@@ -5,7 +5,7 @@ The following guide is for setting up your local environment to contribute to th
## Prerequisites

- Node.js
Gutenberg is a JavaScript project and requires [Node.js](https://nodejs.org/). The project is built using the latest active LTS release of node, and the latest version of NPM. See the [LTS release schedule](https://github.com/nodejs/Release#release-schedule) for details.
Gutenberg is a JavaScript project and requires [Node.js](https://nodejs.org/). The project is built using the version (14.18.1) release of node, and the latest version of NPM. See the [LTS release schedule](https://github.com/nodejs/Release#release-schedule) for details.
Copy link
Member

Choose a reason for hiding this comment

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

At the moment we also use npm v6 rather than the latest one.

@@ -21,10 +21,10 @@ curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.38.0/install.sh | bash
```

Quit and restart terminal
Install the long-term support (lts) version of node.
Install node version (14.18.1).
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep this paragraph as is. See https://nodejs.org/en/about/releases/. Node 14 is considered Maintenance LTS release.

@alexstine
Copy link
Contributor Author

@gziolo What do you think of latest changes? I updated things so I am more generically using v14. I am not sure about keeping that one LTS line the same as I feel it may be a bit confusing. Thoughts?

@gziolo
Copy link
Member

gziolo commented Nov 22, 2021

@mkaz and @ryanwelcher, would you mind helping polishing the documentation changes? 😄

The part where we use 14 as the Node.js version is solid 👍🏻

@alexstine
Copy link
Contributor Author

Thanks @gziolo .

Also, managed to get the checks to pass again. 👍

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's use consistently v6 and v14 for versions based on the Node.js releases page:

https://nodejs.org/en/about/releases/

Screenshot 2021-11-23 at 15 41 02

docs/getting-started/devenv/README.md Outdated Show resolved Hide resolved
docs/getting-started/devenv/README.md Outdated Show resolved Hide resolved
docs/getting-started/devenv/README.md Outdated Show resolved Hide resolved
docs/getting-started/devenv/README.md Outdated Show resolved Hide resolved
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

@alexstine, thank you for wrangling these changes, much appreciated 🙇🏻

@gziolo gziolo merged commit 1714d93 into WordPress:trunk Nov 23, 2021
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 23, 2021
@gziolo
Copy link
Member

gziolo commented Nov 23, 2021

@noisysocks, you might want to cherry-pick this PR to the 5.9 release. It's going to be necessary at some point when we create wp/5.9 branch to ensure that the Node.js version is pinned to v14 forever 😄

@gziolo
Copy link
Member

gziolo commented Nov 23, 2021

I have just tested with the latest trunk and it works like a charm 🎉

@noisysocks noisysocks added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Nov 25, 2021
noisysocks pushed a commit that referenced this pull request Nov 29, 2021
…36744)

* Documentation updates and better NVM support.

* Be more generic about version 14.

* Merge trunk in to branch.

* Apply suggestions from code review

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
@ellatrix ellatrix mentioned this pull request Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants