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

feat: add support for gatsby 5 #852

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add support for gatsby 5 #852

wants to merge 1 commit into from

Conversation

danez
Copy link
Contributor

@danez danez commented Dec 13, 2022

Summary

Part of netlify/pod-compute#309

This allows to specify a version for npm packages to detect the framework.

"detect": {
    "npmDependencies": [{ "name": "gatsby", "version": ">=5.0.0" }],

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.

@netlify
Copy link

netlify bot commented Dec 13, 2022

Deploy Preview for framework-info ready!

Name Link
🔨 Latest commit 62387c6
🔍 Latest deploy log https://app.netlify.com/sites/framework-info/deploys/63988544cf86cb0009a3710f
😎 Deploy Preview https://deploy-preview-852--framework-info.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Dec 13, 2022
- os: windows-latest
node-version: '12.20.0'
node-version: '14.14.0'
Copy link
Contributor Author

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 }
Copy link
Contributor Author

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"
Copy link
Contributor Author

@danez danez Dec 14, 2022

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

@danez danez Dec 14, 2022

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.

@danez danez removed their assignment Jan 12, 2023
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

Successfully merging this pull request may close these issues.

None yet

4 participants