netlify / framework-info

Framework detection utility
MIT License
137 stars 21 forks source link

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

Open ehmicky opened 3 years ago

ehmicky commented 3 years ago

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:

We could use that information to either:

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

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!

ascorbic commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

This is a reasonable approach @erezrokah :+1:

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 commented 2 years ago

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: https://github.com/netlify/framework-info/pull/553. We do specify an engines.node in @sveltejs/kit's package.json already

ascorbic commented 2 years ago

@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 commented 2 years ago

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.