-
Notifications
You must be signed in to change notification settings - Fork 18
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: skip work when gatsby version supports adapters #639
Conversation
✅ Deploy Preview for netlify-plugin-gatsby-demo-v5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for netlify-plugin-gatsby-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Should we add tests for the skipping code? There's nothing that tests the logic for the adapter existing and us skipping the build process.
e2e and unit test cases were added |
exports.runTests = function runTests(env, host) { | ||
jest.setTimeout(10_000) | ||
|
||
async function fetchTwice(url, options) { |
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.
Is this to ensure we hit the origin server?
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 didn't author the test suite so I'm not sure why it was done like that - it was copied from already existing suite in https://github.com/netlify/netlify-plugin-gatsby/tree/main/plugin/test/fixtures/v5/functions-without-gatsby-plugin/e2e-tests
Those test suites don't actually deploy, they are running ntl dev
, and I just changed it to use netlify serve
+
skipped some tests here
netlify-plugin-gatsby/plugin/test/fixtures/v5/with-adapters/e2e-tests/test-helpers.js
Lines 54 to 59 in 884f10d
// `netlify serve` doesn't handle encoded paths same as the platform, | |
// so skipping these tests | |
// `${host}/api/some whitespace`, | |
// `${host}/api/with-äöü-umlaut`, | |
// `${host}/api/some-àè-french`, | |
// encodeURI(`${host}/api/some-אודות`), |
netlify serve
just to show they are working when deployed:
/api/some whitespace
/api/with-äöü-umlaut
/api/some-àè-french
/api/some-אודות
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.
👍🏻
Summary
This makes this build plugin no-op if Gatsby version supports adapters as all the handling will be done by adapter. Work on gatsby adapters can be tracked in gatsbyjs/gatsby#38232
Test plan
Deploy preview generated by #655 (this is just a draft with changes from this PR + adjusting
demo-v5
to use gatsby canary with adapters):And deploy previews on this PR where build plugin continue to do the work:
https://app.netlify.com/sites/netlify-plugin-gatsby-demo-v5/deploys/64ac064285ef5f0008a0bb7c / https://deploy-preview-639--netlify-plugin-gatsby-demo-v5.netlify.app/
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Standard checks:
Adresses https://github.com/netlify/pod-ecosystem-frameworks/issues/543
🧪 Once merged, make sure to update the version if needed and that it was
published correctly.