Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Allow frameworks to specify the minimum Node.js version #288

Open
ehmicky opened this issue Jun 4, 2021 · 11 comments
Open

Allow frameworks to specify the minimum Node.js version #288

ehmicky opened this issue Jun 4, 2021 · 11 comments
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality

Comments

@ehmicky
Copy link
Contributor

ehmicky commented Jun 4, 2021

All Node.js-based frameworks have a specific minimum Node.js version.
At the moment, if a user runs a framework on Netlify with a Node.js version that's too old, the framework will most likely fail at require()-time with some cryptic error message due to some unsupported JavaScript feature or missing Node.js core method. Users might be confused and think this is a bug in Netlify.

Since we detect frameworks, we could use that knowledge to infer the minimum Node.js version. We could do this by adding a new nodeVersionRange field to the frameworks declarations:

  • The value would be a semver range like ^10 || >=12 or >=12.20.0 since some frameworks support Node.js version range that are more complex than just >=version.
  • The minimal version can still be inferred (for example to use as a default Node.js version) using node-semver minVersion() method.

We could use that information to either:

  • Warn users when using the wrong Node.js version, for example by printing a message in the build logs.
  • Choose the default Node.js version run in builds

I would personally favor only warning users and not overriding the Node.js version based on that information because:

  • The logic to decide on the default Node.js version is already quite complicated as mentioned by @ingride
  • Users might not expect that Node.js version unless documented, and documenting it clearly might be difficult if it's framework-specific.
  • When using multiple frameworks, their ranges might be in conflict. This could be fixed by using an intersection, but I am wondering whether that intersection might be empty in some cases.
  • It is consistent with our current strategy for Build plugins. For Build plugins, we not only warn users but fail the build altogether when the Node.js version is incompatible. We do this based on the engines.node field in package.json. This is also consistent with how npm and yarn behave during npm install (although one warns only while the other fails).

That being said, I could also see an argument being made that Netlify users might expect the platform to just use the right Node.js version so things "just work" with their chosen framework.

Note: this discussion was brought up due to Nuxt supporting only Node.js >=12.20.0.

I would be very curious to learn more about what you all think about this idea, especially @ingride @JGAntunes @erezrokah @verythorough @lukeocodes @ascorbic. Thanks!

@ehmicky ehmicky added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jun 4, 2021
@ascorbic
Copy link
Member

ascorbic commented Jun 4, 2021

My concern here is that different versions of frameworks require different versions. Could we read it from the framework's engines.node field, rather than the plugin's?

@ascorbic
Copy link
Member

ascorbic commented Jun 4, 2021

I guess we need to know before we install it, but we could just grab the framework's package.json before installing the right node version

@ehmicky
Copy link
Contributor Author

ehmicky commented Jun 4, 2021

Could we read it from the framework's engines.node field, rather than the plugin's?

Yes, this makes lots of sense. Please note that I meant the above to be specific about frameworks, not plugins. I.e. I meant it to be part of the following files.

Since we declare the npm package names using the npmDependencies field, we could use this to extract the engines.node field. Some frameworks like Nuxt are missing that field, so this would require for them to add that field in their package.json, but this might be best practice anyway.

@erezrokah
Copy link
Contributor

Doesn't npm already prints a warning when you npm install a package that requires a Node.js version that doesn't match the one used? I believe yarn install fails when the version doesn't match (same behavior as npm install --engine-strict).

What is the additional gain in implementing the logic ourselves? The benefit I can see is during site creation so we can recommend users which Node.js version to use. However this might be better solved by always using Node.js LTS as the default Node.js version.

@ascorbic
Copy link
Member

ascorbic commented Jun 7, 2021

As I understand it, we'd do it so we can automatically switch to a supported version. This is why we'd want to know the supported version before running the install.

@erezrokah
Copy link
Contributor

so we can automatically switch to a supported version

Thanks for clarifying @ascorbic. Will using Node.js LTS by default solve this? My suggestion only works if we are OK with assuming all frameworks support Node.js LTS (I think it's quite safe to assume it).

That way we won't have to maintain additional logic. If we do automatically chose a Node.js version based on the framework's engines field we would also need to communicate that to users property.

@ascorbic
Copy link
Member

ascorbic commented Jun 7, 2021

That sounds like the simplest solution. I can't think of a scenario where a new site wouldn't work with LTS, but if there were I think it would be rare enough to say that they could set it manually.

@ehmicky
Copy link
Contributor Author

ehmicky commented Jun 7, 2021

This is a reasonable approach @erezrokah 👍

One potential gotcha is that some frameworks (like Nuxt) do not have an engines.node field. However, that field is best practice and I do not see a reason why a framework would legitimately not want to add it.

@benmccann
Copy link
Contributor

Using Node LTS by default sounds great. SvelteKit just dropped support for Node 12 and we're getting a number of complaints that deploys are failing on Netlify now. I sent a PR that I hope fixes this just for SvelteKit, but I'm not quite sure if it does: #553. We do specify an engines.node in @sveltejs/kit's package.json already

@ascorbic
Copy link
Member

ascorbic commented Nov 2, 2021

@benmccann I made the same mistake, but unfortunately setting the env vars in framework-info doesn't work. With the change of the default build image to Focal, we are at least now defaulting to newer Node for builds. Unfortunately that doesn't apply to the lambda runtime, which still defaults to 12. We have an internal issue which covers this, but I think that right now we should default the lambda runtime to 14 (and hope that Amazon does a new release with 16 soon)

@ascorbic
Copy link
Member

ascorbic commented Nov 2, 2021

I've highlighted this again internally, as I think this is a growing problem. As well as SvelteKit it now affects Gatsby and in some cases Next.js.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

No branches or pull requests

4 participants