fix(pages-router): Add patch for trustHostHeader using res.revalidate#1109
fix(pages-router): Add patch for trustHostHeader using res.revalidate#1109sommeeeer wants to merge 8 commits intoopennextjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 3492764 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
| await this.updateTagsOnSet(key, data, ctx); | ||
| debug("Finished setting cache"); | ||
| } catch (e) { | ||
| console.log("ARE WE HERE"); |
There was a problem hiding this comment.
Yup, nice catch :) thanks
There was a problem hiding this comment.
Seems that while making this I found another bug. You can trigger it like this:
cd examples/pages-router
pnpm openbuild:local && pnpm openbuild:local:startThen in your browser open http://localhost:3002/revalidate/123 and see the error in the terminal.
khuezy
left a comment
khuezy
left a comment
There was a problem hiding this comment.
LGTM but I'll defer to Conico/Victor since I haven't dealt w/ page router in years.
|
Thanks for the review @khuezy |
|
Thanks for the review Nico! I have addressed your comments, and this PR should be ready to ship now. When it comes to this error its coming from here. It won't find that |
| --- pages-api.runtime.prod.js | ||
| +++ pages-api.runtime.prod.js | ||
| @@ -1,5 +1,4 @@ | ||
| - |
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
I think it used to work, trying to understand why not any more
it is no more experimental?
Oh maybe I was only considering cloudflare and we have this and this
Maybe we should dig a little and check if there is a way to factor code for both aws and cloudflare?
WDYT Magnus, you have look at it more deeply
| export const trustHostHeaderRule = ` | ||
| rule: | ||
| kind: member_expression | ||
| pattern: $CONTEXT.trustHostHeader |
There was a problem hiding this comment.
Looks like there are 3 occurences of context.tustHostHeader in this function, what is the rationale for only updating one?
| inside: | ||
| kind: parenthesized_expression | ||
| inside: | ||
| kind: if_statement |
There was a problem hiding this comment.
We should look at making this simpler.
If not possible, it would be nice to add what code we are looking for in the JSDoc
| `; | ||
|
|
||
| // Use correct protocol when doing HEAD fetch for revalidation | ||
| export const headFetchProtocolRule = ` |
There was a problem hiding this comment.
Same comment here, could you please add what snippet we are tryin to match. We might be able to come with a more readable rule too.
| export const patchPagesApiRuntimeProd: CodePatcher = { | ||
| name: "patch-pages-api-runtime-prod", | ||
| patches: [ | ||
| // Trust the host header when invoking `res.revalidate("") from pages router |
There was a problem hiding this comment.
I think it's ok to only comment on trustHostHeaderRule ?
| // Trust the host header when invoking `res.revalidate("") from pages router |
| patchCode: createPatchCode(trustHostHeaderRule), | ||
| versions: ">=15.0.0", | ||
| }, | ||
| // Use correct protocol when doing HEAD fetch for revalidation |
There was a problem hiding this comment.
ditto
| // Use correct protocol when doing HEAD fetch for revalidation |
4 participants
Closes #1102
res.revalidate("/path")is broken on Next 15 and 16 for pages router. The reason is that this part of the revalidate code in Next will be undefined in our case.This code ends up bundled in
/next/dist/compiled/next-server/pages-api.runtime.prod.js. I tried many other ways to solve this that failed:experimental.trustHostHeaderin Next config.trustHostHeadermany places innew NextServer.default({});. Didn't seem to have any effect at all.Not sure why none of these workarounds did the trick, but atleast patching the bundled code in the runtime module seems to do the trick.