fix: don't use ODB for routes that match middleware #1171
Conversation
|
✔️ Deploy Preview for netlify-plugin-nextjs-demo ready! 🔨 Explore the source changes: 2e9ddb4 🔍 Inspect the deploy log: https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/61fa7ff829ab9e0008e549b4 😎 Browse the preview: https://deploy-preview-1171--netlify-plugin-nextjs-demo.netlify.app |
|
✔️ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready! 🔨 Explore the source changes: 2e9ddb4 🔍 Inspect the deploy log: https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/61fa7ff859152d0007755cb4 😎 Browse the preview: https://deploy-preview-1171--netlify-plugin-nextjs-nx-monorepo-demo.netlify.app |
|
✔️ Deploy Preview for netlify-plugin-nextjs-export-demo ready! 🔨 Explore the source changes: 2e9ddb4 🔍 Inspect the deploy log: https://app.netlify.com/sites/netlify-plugin-nextjs-export-demo/deploys/61fa7ff81a639d000747d034 😎 Browse the preview: https://deploy-preview-1171--netlify-plugin-nextjs-export-demo.netlify.app |
|
✔️ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready! 🔨 Explore the source changes: 2e9ddb4 🔍 Inspect the deploy log: https://app.netlify.com/sites/netlify-plugin-nextjs-static-root-demo/deploys/61fa7ff831fed20008fe6468 😎 Browse the preview: https://deploy-preview-1171--netlify-plugin-nextjs-static-root-demo.netlify.app |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
ascorbic
force-pushed
the
mk/middleware-rewrites
branch
4 times, most recently
from
0772ede to
685661c
Compare
ascorbic
force-pushed
the
mk/middleware-rewrites
branch
from
685661c to
c6ff51f
Compare
ascorbic
changed the title
| ? `is one statically-generated or ISR route` | ||
| : `are ${middlewareMatches} statically-generated or ISR routes` | ||
| } that match a middleware function, which means they will always be served from the SSR function and will not use ISR or be served from the CDN. | ||
| If this was not intended, ensure that your middleware only matches routes that you intend to use SSR. |
There was a problem hiding this comment.
cc @KyleBlankRollins for this message 😊
| ...middleware | ||
| .map((route) => { | ||
| const unlocalized = [handlerRewrite(`${route}`), handlerRewrite(`${route}/*`)] | ||
| if (i18n?.locales?.length > 0) { |
There was a problem hiding this comment.
| if (i18n?.locales?.length > 0) { | |
| if (i18n?.locales?.length) { |
There was a problem hiding this comment.
Explicitly testing against zero is in the linting rules
src/helpers/redirects.ts
Outdated
| .map((route) => { | ||
| const unlocalized = [handlerRewrite(`${route}`), handlerRewrite(`${route}/*`)] | ||
| if (i18n?.locales?.length > 0) { | ||
| const localized = i18n?.locales?.map((locale) => [ |
There was a problem hiding this comment.
| const localized = i18n?.locales?.map((locale) => [ | |
| const localized = i18n.locales.map((locale) => [ |
Since the above check prevents it, you don't need to have the optional check here
src/helpers/redirects.ts
Outdated
| @@ -65,6 +71,7 @@ export const generateStaticRedirects = ({ | |||
| } | |||
| } | |||
|
|
|||
| // eslint-disable-next-line max-lines-per-function | |||
There was a problem hiding this comment.
[sand] This function is indeed starting to get pretty complex. Could you split it out?
There was a problem hiding this comment.
I'\ve split out the various bits
| } from './utils' | ||
|
|
||
| const matchesMiddleware = (middleware: Array<string>, route: string): boolean => |
There was a problem hiding this comment.
| const matchesMiddleware = (middleware: Array<string>, route: string): boolean => | |
| const matchesMiddleware = (middleware: Array<string> | undefined, route: string): boolean => |
[sand] Or optional? I see you check the existence in the line below
There was a problem hiding this comment.
Looks like middleware is always defined, because getMiddleware defaults to [], so I'll just remove the optional chain check
src/helpers/files.ts
Outdated
| export const getMiddleware = async (publish: string): Promise<Array<string>> => { | ||
| const manifestPath = join(publish, 'server', 'middleware-manifest.json') | ||
| if (existsSync(manifestPath)) { | ||
| const manifest = await readJson(manifestPath) |
There was a problem hiding this comment.
No error handling? It looks like readJSON also allows you to pass it { throws: false } so it doesn't throw if it fails to read the JSON.
There was a problem hiding this comment.
Thanks for tagging me, @tiffanosaurus! @ascorbic, I have a couple edits, but wouldn't consider them blocking.
|
@tiffanosaurus Added types, thanks! |
Summary
Routes that match middleware need to be served via SSR, because the middleware is run at the origin. This PR ensures that any routes that matches mdidleware is not served by an ODB, which was causing errors where results were persisted including the middleware.
Test plan
x-middleware-dateinserted by middleware. Ensure that the date is up to date. Also ensure thatx-nf-render-modeisssr.Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Fixes #1168, Fixes #1114
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.