feat: add Next 13 request header mutation to middleware #1866
Conversation
✅ 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 netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
github-actions
bot
added
the
type: feature
✅ 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 next-i18next-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 next-plugin-canary 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 next-plugin-edge-middleware 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 ep-next-middleware-request-mutation ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
package.json
Outdated
| @@ -103,6 +103,9 @@ | |||
| "!**/test/fixtures/**", | |||
| "!**/test/sample/**" | |||
| ], | |||
| "moduleNameMapper": { | |||
| "https://deno.land/x/[email protected]": "<rootDir>/test/mockedDenoModules/html_rewriter" | |||
| }, | |||
There was a problem hiding this comment.
Can we use deno test instead of Jest for EF stuff
There was a problem hiding this comment.
There's an example in the edge router PR: https://github.com/netlify/next-runtime/pull/1451/files#diff-c02e108e555d63f8e66ff0b099106e84ef940d69b5cb0e1d2c12b2f79c1f6322
There's a workflow for it in there too
| // * https://github.com/vercel/next.js/blob/68d06fe015b28d8f81da52ca107a5f4bd72ab37c/packages/next/server/next-server.ts#L1918-L1928 | ||
| // * https://github.com/vercel/next.js/blob/43c9d8940dc42337dd2f7d66aa90e6abf952278e/packages/next/server/web/spec-extension/response.ts#L10-L27 | ||
| export function updateModifiedHeaders(response: Response) { | ||
| const overriddenHeaders = response.headers.get('x-middleware-override-headers') || '' |
There was a problem hiding this comment.
No need for the fallback if you're checking for falsiness right away
| const oldHeaderKey = 'x-middleware-request-' + header | ||
| const headerValue = response.headers.get(oldHeaderKey) || '' | ||
|
|
||
| response.headers.set(header, headerValue) |
There was a problem hiding this comment.
We don't want to set these in the response - it's the request that needs them, so we should be calling this before calling context.next() and setting them on req instead
There was a problem hiding this comment.
The request headers appear to be fine in that they don't have the x-middleware-request string prefixed to every header, nor does it contain the x-middleware-override-headers header - it's the result.response headers that have them and are not having them cleaned up.
Would be happy to pair on this tomorrow though as there's likely more insight that you have that I'm missing
There was a problem hiding this comment.
It's not setting them on the request though: https://deploy-preview-1866--next-plugin-edge-middleware.netlify.app/api/hello
They're not meant to be on the response at all, so they need to be stripped and not added back in. They're just meant to be in the request that sent to the origin.
|
@ascorbic I don't mind the early feedback, thank you! 😄 |
|
This is the failing test for this: https://cloud.cypress.io/projects/yn8qwi/runs/1235/overview/3ed1f6bf-da05-44e1-bc69-f5f2451bed7c |
…fy/next-runtime into ep/support-request-header-mutation
Summary
Test plan
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.