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

Specify Node version 16 #40

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Specify Node version 16 #40

merged 1 commit into from
Oct 21, 2022

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Oct 21, 2022

Description of proposed changes

It's recommended to set the Node version explicitly¹, otherwise the build server chooses a default which can change unexpectedly. For example, this change was prompted because Heroku recently switched from 16 to 18. Auspice doesn't work on 18 yet², so we need to use 16.

¹ https://devcenter.heroku.com/articles/nodejs-support#specifying-a-node-js-version
² nextstrain/auspice#1553

Related issue(s)

N/A

Testing

  • Manually inspect Heroku build log (ref)
  • Preview app opens

@victorlin victorlin self-assigned this Oct 21, 2022
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-us-victorlin-us-cbdyye October 21, 2022 17:03 Inactive
Comment on lines +6 to +8
"engines": {
"node": "16.x"
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Build output:

-----> Building on the Heroku-22 stack
-----> Using buildpack: heroku/nodejs
-----> Node.js app detected

...

-----> Installing binaries
       engines.node (package.json):  16.x
       engines.npm (package.json):   unspecified (use default)
       
       Resolving node version 16.x...
       Downloading and installing node 16.18.0...
       Using default npm version: 8.19.2

This looks good, but I noticed that if the npm version is not specified, it defaults to the latest version compatible with the node version. The only potential issue I can think of is having different package-lock.json files due to developers having npm <=6 (lockfile v1) vs. >=7 (lockfile v2). I don't think this can be enforced through the engines alone. Even when specifying "npm": ">=7", a local environment with Node.js v14.18.3 and npm 6.14.15 does not produce any warnings when running npm ci. On the other hand, a local environment with Node.js 18 shows a warning as expected:

npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '16.x' },
npm WARN EBADENGINE   current: { node: 'v18.10.0', npm: '8.19.2' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '^12 || ^14 || ^16', npm: '>=6.9' },
npm WARN EBADENGINE   current: { node: 'v18.10.0', npm: '8.19.2' }
npm WARN EBADENGINE }

So I think it's fine to leave this change as-is (node version enforcement), and npm version enforcement can come later.

It's recommended to set the Node version explicitly¹, otherwise the
build server chooses a default which can change unexpectedly. For
example, this change was prompted because Heroku recently switched from
16 to 18. Auspice doesn't work on 18 yet², so we need to use 16.

¹ https://devcenter.heroku.com/articles/nodejs-support#specifying-a-node-js-version
² nextstrain/auspice#1553
@victorlin victorlin force-pushed the victorlin/use-node-16 branch from 17a5a24 to 18390e4 Compare October 21, 2022 18:17
@victorlin victorlin changed the title Specify Node version 16 in package.json Specify Node version 16 Oct 21, 2022
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-us-victorlin-us-cbdyye October 21, 2022 18:17 Inactive
@victorlin victorlin merged commit f693557 into master Oct 21, 2022
@victorlin victorlin deleted the victorlin/use-node-16 branch October 21, 2022 18:21
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.

2 participants