-
Notifications
You must be signed in to change notification settings - Fork 87
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
chore: upgrade to next v13.4 #2080
Conversation
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@@ -48,6 +49,16 @@ const fetch /* type {typeof globalThis.fetch} */ = async (url, init) => { | |||
} | |||
} | |||
|
|||
// Turbopack aliases "Buffer" to a module import, so we need to provide a shim for that | |||
if (typeof require === 'undefined') { |
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.
Since Next.js 13.4 is stable, we should switch to that version in this PR and continue with that. |
…-nextjs into tn/nextjs-v13.3.4
return <p>Node!</p> | ||
} | ||
|
||
export const runtime = 'edge' |
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.
nit: new line at end of file
} | ||
return <p>Node!</p> | ||
} |
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.
nit: new line at end of file
@@ -929,32 +915,6 @@ describe('app dir', () => { | |||
}) | |||
}) | |||
|
|||
describe('previewData function', () => { |
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.
Are these test removals because the test suite in Next.js removed them?
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.
no, we need to overhaul our tests because our current scaffolding doesn't work with many of the new app-dir tests - i think it's important to get a basic level of support shipped and then work on getting these tests in, but shout if you disagree
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.
Sounds good. I created #2161 for this.
const endTime = Date.now() | ||
const duration = endTime - startTime | ||
// Each part takes 5 seconds so it should be below 10 seconds | ||
// Using 7 seconds to ensure external factors causing slight slowness don't fail the tests | ||
expect(duration < 7000).toBe(true) | ||
expect(duration).toBeLessThan(10000) |
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.
Did this get slower, hence the bump to less than 10000?
@@ -71,7 +81,7 @@ beforeAll(() => { | |||
}) | |||
|
|||
describe('the netlify next server', () => { | |||
it('does not revalidate a request without an `x-prerender-revalidate` header', async () => { | |||
it.skip('does not revalidate a request without an `x-prerender-revalidate` header', async () => { |
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.
Do we have issues created for the skipped tests?
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.
this is all part of the on-demand revalidation that needs to be completed reworked 😬
i've left the tests there in case we need to reference them in future but it'll need to be revisited when we look at on-demand revalidation next
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.
This looks great. Thanks for answering my questions @orinokai.
🚢 🚢
🚢
🚢
@@ -929,32 +915,6 @@ describe('app dir', () => { | |||
}) | |||
}) | |||
|
|||
describe('previewData function', () => { |
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.
Sounds good. I created #2161 for this.
This merge seams to breack backward compatibility. Now Next version running version less then version 13.4.0 breaks and sites return 500. I downgraded my site yesterday to 13.3.0 and the site work expect for hard refresh on navigation. Today after the merge website just gives me 500. |
@bentrynning While this PR was merged, it wasn't released yet. So unless you somehow use GitHub as a source, you won't use this just yet. |
Summary
next
dependencies to 13.4Test plan
Ensure current tests pass
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Fixes https://github.com/netlify/pod-ecosystem-frameworks/issues/473
Fixes https://github.com/netlify/pod-ecosystem-frameworks/issues/489
BEGIN_COMMIT_OVERRIDE
fix: ensures compatibility with Next.js 13.4 (support for some features still en-route)
fix: uses pre-bundled React modules for App Router paths
END_COMMIT_OVERRIDE