Closed Bug 1443256 Opened 6 years ago Closed 2 years ago

Add top-level .node-version and .nvmrc files for developer convenience

Categories

(Developer Infrastructure :: Developer Environment Integration, enhancement)

3 Branch
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: nalexander, Unassigned)

References

Details

JoeH suggested that we should make life smooth for Node developers that use nvm/n/avn to manage Node versions, and I think that's a great idea.

Right now, there are some restrictions on the versions of Node that work with ESLint -- but I don't know them all.  This ticket tracks baking those restrictions into top-level (sibling to https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/package.json) .node-version and .nvmrc files.
Standard8: do you see any issue with this?  If IUUC, these files will be ignored by users that don't use these tools, so there shouldn't be significant harm to existing users caught in the cross-fire.

Can you suggest what to pin to?  Probably the versions we run in automation are the safest (that's 6.9.1, right? -- https://searchfox.org/mozilla-central/source/tools/lint/eslint/setup_helper.py#20), but I think consumers in tree (devtools/client/webconsole) are actually using 8.9.
Flags: needinfo?(standard8)
We should be constantly updating the version in automation as well.  Please only pin to the minor version (e.g. 8.9), to allow for security updates.  

Also please plan to update this to 10.x soon after 10 is released.
(In reply to Nick Alexander :nalexander from comment #1)
> Standard8: do you see any issue with this?  If IUUC, these files will be
> ignored by users that don't use these tools, so there shouldn't be
> significant harm to existing users caught in the cross-fire.

I think given we're starting to require node as part of the build / for main tools, it is probably worth doing something like this.

An alternative could also be that we have some sort of version check on node based commands (e.g. ./mach lint -l eslint, eventually ./mach build), to enforce that we have the correct min version.

This would be similar to what we do with the rust version.

The other option is we do this progressive - add the suggested files in for now, and then add build automation requirements once we have a hard-requirement for ./mach build.

Maybe we should ask gps for his thoughts on what's best?

What does .node-version do / interact with? I couldn't find that through searching.

> Can you suggest what to pin to?  Probably the versions we run in automation
> are the safest (that's 6.9.1, right? --
> https://searchfox.org/mozilla-central/source/tools/lint/eslint/setup_helper.
> py#20), but I think consumers in tree (devtools/client/webconsole) are
> actually using 8.9.

Personally, I think we should go with 8.9.x. I'd semi like to restrict that to 8.9.4 and above to help increase the npm version as well [1], but I know MozillaBuild hasn't been updated yet for instance.

For automation, I'm not too worried about that we should be able to update it easily as I think it is just the ESLint items using it, and I know they work fine on 8.9.x as that's what I use locally. I'm quite happy to start investigating this for ESLint.

Also, we said 8.9.x in the intent to require node document, and that version has been out for 10 months now, so I think it is reasonable.

(In reply to Joe Hildebrand  [:hildjj] (UTC-6) from comment #2)
> Also please plan to update this to 10.x soon after 10 is released.

What's the reason for that? I'm a bit concerned that Linux especially won't necessarily have the relevant packages updated/available straight away.



[1] There's been improvements to the 5.x series, and npm 5.6.0 is the better one so far, and there's other things I'd like to get if developers are on the 5.x series.
Flags: needinfo?(standard8)
(In reply to Mark Banner (:standard8) from comment #3)
> An alternative could also be that we have some sort of version check on node
> based commands (e.g. ./mach lint -l eslint, eventually ./mach build), to
> enforce that we have the correct min version.

At least a warning would be nice.

> What does .node-version do / interact with? I couldn't find that through
> searching.

As is traditional in the Node world, there's more than one project that people are passionate about.

.node-version drives avn: https://github.com/wbyoung/avn
.nvmrc drives nvm: https://github.com/creationix/nvm#nvmrc
n also matters: https://github.com/tj/n

nvm is the old standby, tried and true.  But it mostly only works if you use bash or zsh.  n is the new hotness, written in node. avn will auto-switch node versions based on your current directory.  It's new-fangled and has plugins for nvm and n.  nodebrew also exists, but since it's written in perl I haven't tried it.

There are almost certainly other choices I haven't heard about yet.

I have recently switched to n/avn/avn-n from nvm.

> (In reply to Joe Hildebrand  [:hildjj] (UTC-6) from comment #2)
> > Also please plan to update this to 10.x soon after 10 is released.
> 
> What's the reason for that? I'm a bit concerned that Linux especially won't
> necessarily have the relevant packages updated/available straight away.

I was unclear.  Once 10.x is the actual LTS and the major distros have shipped support for it would be the time to consider this.  There are always new features that would be nice to use as well as performance improvements.  I don't want to get to 5 years from now and not have continuously upgraded when it was prudent to have done so.
Depends on: 1443547
(In reply to Mark Banner (:standard8) from comment #3)
> (In reply to Nick Alexander :nalexander from comment #1)
> > Standard8: do you see any issue with this?  If IUUC, these files will be
> > ignored by users that don't use these tools, so there shouldn't be
> > significant harm to existing users caught in the cross-fire.
> 
> I think given we're starting to require node as part of the build / for main
> tools, it is probably worth doing something like this.
> 
> An alternative could also be that we have some sort of version check on node
> based commands (e.g. ./mach lint -l eslint, eventually ./mach build), to
> enforce that we have the correct min version.
> 
> This would be similar to what we do with the rust version.
> 
> The other option is we do this progressive - add the suggested files in for
> now, and then add build automation requirements once we have a
> hard-requirement for ./mach build.

Roger that -- I have patches locally that check for Node and NPM at configure time and ensure that both are present and accepted versions -- just like Rust and Python.

> Maybe we should ask gps for his thoughts on what's best?
> 
> What does .node-version do / interact with? I couldn't find that through
> searching.
> 
> > Can you suggest what to pin to?  Probably the versions we run in automation
> > are the safest (that's 6.9.1, right? --
> > https://searchfox.org/mozilla-central/source/tools/lint/eslint/setup_helper.
> > py#20), but I think consumers in tree (devtools/client/webconsole) are
> > actually using 8.9.
> 
> Personally, I think we should go with 8.9.x. I'd semi like to restrict that
> to 8.9.4 and above to help increase the npm version as well [1], but I know
> MozillaBuild hasn't been updated yet for instance.

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1443545 to track upgrading.
 
> For automation, I'm not too worried about that we should be able to update
> it easily as I think it is just the ESLint items using it, and I know they
> work fine on 8.9.x as that's what I use locally. I'm quite happy to start
> investigating this for ESLint.

And I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1443547 to track upgrading the lint Docker image.
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure

node and npm are now managed by the build system and automatically updated, they typically live in the .mozbuild directory. There are also warnings in place if the used versions are out-of-date.

Given that, I think we no longer need to add other methods.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.