Framework: Pin nvmrc to specific current LTS#22236
Conversation
aduth
added
the
Framework
|
Size Change: +311 B (0%) Total Size: 824 kB
ℹ️ View Unchanged
|
chrisvanpatten
left a comment
chrisvanpatten
left a comment
There was a problem hiding this comment.
Makes sense to me!
I also did some quick digging to compare this with tying to a specific LTS "codename" (e.g. lts/erbium) and it turns out internally nvm converts those names to the version number anyway, so there's no practical difference. If anything, this is clearer because the codenames aren't often used.
|
One thought: is it worth documenting that this is expected to reflect the latest LTS release? Unfortunately it doesn't look like |
It would be nice if we could include a comment in the file, yeah. I dug a bit into the documentation and source code, and can't seem to find any indication of a syntax for supporting comments. Where do you imagine it might make sense to document this otherwise? We do already have the expectation outlined in the "Getting Started" guide, but this is just the general expectation, not anything about
|
|
How about something like this:
|
|
It reads nicely to me. My only hesitation is that this document is the entry point for new contributors, and is already pretty over-encumbered with information. While it's all certainly useful, some of it isn't particularly relevant for a new contributor's first steps (e.g. working in old branches). |
|
@aduth Maybe if it were formatted more like this: Gutenberg is built using the latest active LTS release of Node, along with the latest version of NPM. The easiest way to install and manage node and NPM (on macOS, Linux, or Windows 10 with the Linux Subsystem) is by using nvm. Once nvm is installed, you can install the correct version of Node by running
... I also simplified the language a bit to reflect that this is intended for new contributors. |
|
Also related: https://core.trac.wordpress.org/ticket/50094 |
Thanks @chrisvanpatten . It seems like a nice balance of information. I think it also does a good job to consolidate / highlight the expected versions of Node/NPM in that introductory sentence. Separately, something about the existing "Python requirement" paragraph stands out to me as disjointed. I don't think it's directly related to what's being modified here, so maybe consider as a separate task. |
3 participants
Related Slack discussion (link requires registration): https://wordpress.slack.com/archives/C02QB2JS7/p1588794429373800
This pull request seeks to update
.nvmrcto designate the actual current LTS version. This is intended to future-proof builds which occur when checking out an older version of the branch. Notably, this is an issue today when checking out old versions of Gutenberg, where we had used an older version of thenode-sassdependency which only works in e.g. Node 10 or earlier. If, at the time,.nvmrchad specified10as the Node version of the project, it would not be as much an issue†. With these changes, we can at least try to correct this problem moving forward.Note that the primary downside here is that we will need to manually update this value over time, as the LTS changes. Based on the release schedule, this should be expected to occur once every year (each October).
Testing Instructions:
Running
nvm installshould install Node v12.16.3:Follow-up tasks:
† This only really helps if one knows to run
nvm install && nvm useafter checking out an older branch / commit. Ideally we can make this more seamless, or at least more noisy as part of checks prior to any actions we expect to depend on this behavior.Example ideas:
post-checkoutgit hook to runnvm install && nvm useprebuildcheck-enginesstepenginesinpackage.json?.nvmrcincheck-engines?