-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add support for gatsby 5 #852
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for framework-info ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
- os: windows-latest | ||
node-version: '12.20.0' | ||
node-version: '14.14.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 12 was dropped.
const scripts = getScripts(packageJson) | ||
return { npmDependencies, scripts } | ||
return { npmDependencies, npmDependenciesVersions, scripts } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm exporting all the versions as Map<dep-name, version>
in addition to the array of dependency names, because otherwise we would have todo Object.keys()
a lot everywhere, and I wasn't sure about if the performance would suffer.
It is not ideal to kinda return the same info in different formats, but I think that is the compromise here.
"GATSBY_LOGGER": "yurnalist", | ||
"GATSBY_PRECOMPILE_DEVELOP_FUNCTIONS": "true", | ||
"AWS_LAMBDA_JS_RUNTIME": "nodejs18.x", | ||
"NODE_VERSION": "18" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables seem to only be used in the CLI and I'm not even certain about this. I haven't tested this because I think what we want cannot be achieved with setting NODE_VERSION. What we want is an explicit setting minimumRequiredNodeVersion
, which I did not add in this PR, as this PR is only about splitting gatsby into two configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of needing to create a new framework for each version. It feels hacky and hard to maintain. What happens when Gatsby 6 is released? Do we create a new one, or rename it to gatsby5plus? The approach of the plugins repo of allowing an array of versions, each with their own rules, seems to be better, though would be more of a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer your question - I think you are right, and these are only used in ntl dev
, so AWS_LAMBDA_JS_RUNTIME
is also incorrect here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just followed the pattern that existed for other frameworks as everything else would be a big change.
I think that it wouldn't even be a breaking change, just a lot of internal changes. The returned result could stay the same.
What I'm thinking of is something like this (simplified)
{
"id": "gatsby5",
"name": "Gatsby",
"detect": {
"npmDependencies": ["gatsby"],
"excludedNpmDependencies": [],
"configFiles": ["gatsby-config.js"]
},
"overrides": [
{
"detect": {
"npmDependencies": [{name: "gatsby", version: ">99"}],
},
"dev": {
"port": 1111,
},
}
],
"dev": {
"port": 8888,
},
}
So in this example it would change the dev port if a certain version of the framework is used, but inherit everything else from the initial config. The structure in the processed end result would be exactly the same, at least for the framework detection.
Summary
Part of netlify/pod-compute#309
This allows to specify a version for npm packages to detect the framework.
With this new setting, we can define a new config for gatsby 5 and this is the first step of zero-config gatsby 5.
Even though framework-info has env variables that are defining the node version, I think we will still need to maybe add a new
minimumNodeVersion
to the mix, because the environment variables are not used in build itself afaics.