fix: adjust middleware json data rewrite to work with recent next@canary #2734
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Package size report -0%↓
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
added
the
test all versions
pieh
force-pushed
the
michalpiechowiak/frb-1545-investigate-integration-test-failures-due-to-recent-pr
branch
2 times, most recently
from
5242348 to
2484e5e
Compare
pieh
force-pushed
the
michalpiechowiak/frb-1545-investigate-integration-test-failures-due-to-recent-pr
branch
from
2484e5e to
f5bb340
Compare
pieh
force-pushed
the
michalpiechowiak/frb-1545-investigate-integration-test-failures-due-to-recent-pr
branch
from
87cedbb to
4289aa2
Compare
pieh
changed the title
pieh
commented
Jan 3, 2025
| @@ -14,6 +14,8 @@ import { | |||
| normalizeLocalePath, | |||
| normalizeTrailingSlash, | |||
| relativizeURL, | |||
| removeBasePath, | |||
| rewriteDataPath, | |||
There was a problem hiding this comment.
Fun fact - we had rewriteDataPath function already but we never used it
pieh
commented
Jan 3, 2025
| @@ -180,14 +182,16 @@ export const buildResponse = async ({ | |||
| } | |||
|
|
|||
| if (isDataReq) { | |||
There was a problem hiding this comment.
As far as I can tell - this will only be truthy for pages router json requests
opennextjs-netlify/edge-runtime/lib/response.ts
Lines 126 to 127 in a1d859d
App router RSC requests don't seem to have that header set so this should be safe to do
There was a problem hiding this comment.
I might be wrong, but it looks like next uses getRequestMeta(params.req, 'isNextDataReq') for isDataRequest? There's a chance we could do the same.
https://github.com/vercel/next.js/blob/canary/packages/next/src/server/request-meta.ts#L183-L205
There was a problem hiding this comment.
This was my initial attempt - f5bb340 but to make it work, something in middleware handling (edge function) would need to signal to origin (lambda) that this meta data should be set - in that commit I just made the previous query param work again (if it was present I was setting that meta data), but this felt wrong given that Next.js explicitly removed support for that. I'm not exactly sure how it works on Vercel - I'm clearly missing something here, but I don't think attempting to set those meta tags directly is the right way to go - maybe figuring out how that meta data is meant to be set would be the best solution here - I'll try to look if I can figure that out
There was a problem hiding this comment.
Ok, so the part of code that is setting the meta data on requests (~https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-utils/resolve-routes.ts) ... but it only happens when it is _next/data request to begin with, so because we were using canonical path before we never hit this code path and we just relied on fact that we could "externally" force it with query param.
So current fix (at least in terms of the approach of using data url instead of trying to add the internal meta data on request ourselves) seems like generally correct approach to me, because we now hit the code path that is setting that internal meta data and added e2e test show it working for all Next.js versions that we test
There was a problem hiding this comment.
Thanks for looking into it even more deeply!
pieh
commented
Jan 3, 2025
Comment on lines
+56
to
+71
| test('json data rewrite works', async ({ middlewarePages }) => { | ||
| const response = await fetch(`${middlewarePages.url}/_next/data/build-id/sha.json`, { | ||
| headers: { | ||
| 'x-nextjs-data': '1', | ||
| }, | ||
| }) | ||
|
|
||
| expect(response.ok).toBe(true) | ||
| const body = await response.text() | ||
|
|
||
| expect(body).toMatch(/^{"pageProps":/) | ||
|
|
||
| const data = JSON.parse(body) | ||
|
|
||
| expect(data.pageProps.message).toBeDefined() | ||
| }) |
There was a problem hiding this comment.
Using already existing setup in one of our fixtures
opennextjs-netlify/tests/fixtures/middleware-pages/middleware.js
Lines 37 to 40 in c3e328c
and
opennextjs-netlify/tests/fixtures/middleware-pages/pages/shallow.js
Lines 37 to 43 in c3e328c
mrstork
approved these changes
Jan 3, 2025
pieh
deleted the
michalpiechowiak/frb-1545-investigate-integration-test-failures-due-to-recent-pr
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adjusts middleware handling to use json data urls and not route paths with
__nextDataReqquery param (that no longer works with recent next@canary after vercel/next.js#74100) for middleware json data rewritesInitial previous attempt description below (this felt the quickest "hot fix" but it seemed really wrong to do so):
This is attempting to address changes made in vercel/next.js#74100 by keeping previous query param "working".This feels like wrong approach to me, but I'm not quite sure how this should be fixed properly. In our middleware handling we are relying on this search param:opennextjs-netlify/edge-runtime/lib/response.ts
Lines 182 to 186 in c3e328c
Maybe instead we should transform pathname into json data path instead of keeping it as cannonical route path with added query param, but comment in existing code gives me a pause - I'm not sure if it's about our own handling or it's about Next handling in general.This feels like quickest (and quite hacky ...) way to keep middleware rewrites for data routes working, so maybe this could be considered stop-gap solution that will do for now, until more investigation can happen.Documentation
Tests
Added e2e test (we only had integration tests really and those were not checking all parts of handling request - one test was checking origin - literally if using
__nextDataReqsearch param results in data response (I did remove this one as it would no longer work withnext@canaryand because we are not relying on this behavior anymore) and the other ones were testing just middleware part - I did update those)Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRB-1545/investigate-integration-test-failures-due-to-recent-pr-changes