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

NPM package config: set Node version constraint #1440

Merged

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Feb 24, 2023

This formally encode the Docsy project constraint that we officially support only the active LTS version of Node:

docsy/.nvmrc

Line 1 in 6f796f9

lts/*

@chalin chalin requested review from LisaFC and geriom February 24, 2023 14:04
Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

LGTM.

In this context, I would propose to also add (and pin) the node version in netlify.toml, thus getting more reproducible builds:

[build.environment]
NODE_VERSION = "18.14.2"
GO_VERSION = "1.19.5"
HUGO_THEME = "repo"

@chalin chalin force-pushed the chalin-im-node-vers-constraint-2023-02-24 branch from 2eb3e5e to e0de1fa Compare February 24, 2023 18:52
@chalin
Copy link
Collaborator Author

chalin commented Feb 24, 2023

It is already constrained via https://github.com/google/docsy/blob/main/.nvmrc, which is better than adding a constraint in the Netlify config, as pointed out in https://docs.netlify.com/configure-builds/manage-dependencies/#node-js-and-javascript:

image

At this point I'm not looking to pin Node to a specific version. Any version of the Active LTS is what we're meant to support.

Let's keep things as dry as we can. The engines.node constraint is a lower bound -- if I could, I'd put the equivalent of lts/* but that doesn't seem possible. As you know we've had problems in the past with folks reporting issues that arose because they were using a version of Node that was far too old. This will help improve on that scenario.

@chalin
Copy link
Collaborator Author

chalin commented Feb 24, 2023

Thanks for the feedback @deining. I'll merge this as is for now.

@chalin chalin merged commit 36c7405 into google:main Feb 24, 2023
@chalin chalin deleted the chalin-im-node-vers-constraint-2023-02-24 branch February 24, 2023 19:00
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