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

feat: support for Gatsby v5 #547

Merged
merged 17 commits into from Dec 16, 2022
Merged

feat: support for Gatsby v5 #547

merged 17 commits into from Dec 16, 2022

Conversation

ericapisani
Copy link
Contributor

@ericapisani ericapisani commented Nov 29, 2022

Summary

Include .nvmrc files in both demos to specify minimum node versions in order to make it easy to work with now that there are different minimum node versions for the 2 demos.

I opted to create a 2nd demo site as we will have to continue to support v4 for the time being (will be until end of 2023 as that is how long Gatsby will maintain it, though will only have maintenance changes done to it).

I've also updated the minimum version of react to 18 in e2e tests that have gatsby@next being installed. We may want to consider having more e2e tests to ensure v4 functionality is maintained, but at the moment e2e tests take a while to run and I wanted to have that discussion with the broader team.

Dependant PR: netlify/gatsby-plugin-netlify#203

Migration guide from Gatsby - https://www.gatsbyjs.com/docs/reference/release-notes/migrating-from-v4-to-v5/

Test plan

  1. Visit the Deploy Preview - https://ep-gatsby-v5--ep-gatsby-v5.netlify.app
  • Confirm that the SSR and DSG pages continue to work as expected
  1. Visit the v4 deploy preview - https://deploy-preview-547--netlify-plugin-gatsby-demo.netlify.app/
  • Confirm the links work as expected
  • Visit the api/users/123 to confirm that the page doesn't crash (this confirms the updated import logic here works as expected

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Fixes https://github.com/netlify/pod-ecosystem-frameworks/issues/269 , netlify/gatsby-plugin-netlify#195

Standard checks:

  • Check the Deploy Preview's Demo site for your PR's functionality
  • Add docs when necessary

@ericapisani ericapisani requested a review from a team November 29, 2022 17:21
@ericapisani ericapisani self-assigned this Nov 29, 2022
@netlify
Copy link

netlify bot commented Nov 29, 2022

Deploy Preview for ep-gatsby-v5 canceled.

Name Link
🔨 Latest commit da7e52f
🔍 Latest deploy log https://app.netlify.com/sites/ep-gatsby-v5/deploys/6398916232320a0008f0b17d

@netlify
Copy link

netlify bot commented Nov 29, 2022

Deploy Preview for netlify-plugin-gatsby-demo canceled.

Name Link
🔨 Latest commit da7e52f
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-gatsby-demo/deploys/639891624f30ac0008837dfc

demo-v5/package.json Outdated Show resolved Hide resolved
@ericapisani ericapisani changed the title chore: add demo site for Gatsby v5 feat: support for Gatsby v5 Nov 30, 2022
@ericapisani
Copy link
Contributor Author

Based on conversations with our build team, this PR is blocked at the moment pending full support for node 18 in our build systems (see this slack thread) and some framework detection enhancements.

want to test them against both v4 and v5 of Gatsby
@ericapisani ericapisani marked this pull request as draft December 13, 2022 13:52
also update the github workflow yaml as it was using an invalid
value
the parallelization needs to happen within the GH containers instead
@ericapisani ericapisani marked this pull request as ready for review December 13, 2022 15:33
Copy link
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Ran through the two deploy previews and everything is behaving as expected.

For

Visit the api/users/123 to confirm that the page doesn’t crash (this confirms the updated import logic here works as expected

It doesn’t crash but it returns a message Not found. Is that expected?

.github/workflows/test.yml Show resolved Hide resolved
@ericapisani
Copy link
Contributor Author

@nickytonline

It doesn’t crash but it returns a message Not found. Is that expected?

Yes that's correct. Prior to the logic introduced here, you would see a function crashing error rather than a 'not found'

Copy link
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Thanks for answering my questions @ericapisani! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants