fix: allow some recursion for middleware subrequests #60615

juliusmarminge · 2024-01-13T22:35:03Z

This alters the behavior of the subrequest check to allow for 5 recursive calls to match Vercel production, ref Slack thread.

Note

Currently limited by fetches having to forward the subrequest header for each request which isn't ideal. Need some assistant on how to access the request in the module context fetch override.
No forwarding:

With forwarding:

ijjk · 2024-01-13T22:35:27Z

Allow CI Workflow Run

approve CI run for commit: a116cbd

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2024-01-13T22:35:27Z

Allow CI Workflow Run

approve CI run for commit: 5c75071

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

packages/next/src/server/web/sandbox/sandbox.ts

ijjk · 2024-02-05T22:05:19Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
buildDuration	11.9s	11.9s	N/A
buildDurationCached	6s	5.3s	N/A
nodeModulesSize	196 MB	196 MB	⚠️ +7.98 kB
nextStartRea..uration (ms)	410ms	409ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
3f784ff6-HASH.js gzip	53.4 kB	53.4 kB	✓
423.HASH.js gzip	185 B	181 B	N/A
68-HASH.js gzip	29.7 kB	29.7 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	240 B	240 B	✓
main-HASH.js gzip	31.8 kB	31.8 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	✓
Overall change	101 kB	101 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
_app-HASH.js gzip	194 B	195 B	N/A
_error-HASH.js gzip	182 B	181 B	N/A
amp-HASH.js gzip	502 B	501 B	N/A
css-HASH.js gzip	320 B	322 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	256 B	N/A
head-HASH.js gzip	350 B	349 B	N/A
hooks-HASH.js gzip	368 B	369 B	N/A
image-HASH.js gzip	4.19 kB	4.18 kB	N/A
index-HASH.js gzip	257 B	256 B	N/A
link-HASH.js gzip	2.61 kB	2.61 kB	N/A
routerDirect..HASH.js gzip	310 B	311 B	N/A
script-HASH.js gzip	384 B	383 B	N/A
withRouter-HASH.js gzip	306 B	308 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	106 B	106 B	✓

Client Build Manifests

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
_buildManifest.js gzip	483 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
index.html gzip	529 B	526 B	N/A
link.html gzip	540 B	537 B	N/A
withRouter.html gzip	524 B	521 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
edge-ssr.js gzip	94 kB	94 kB	N/A
page.js gzip	149 kB	149 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
middleware-b..fest.js gzip	621 B	626 B	N/A
middleware-r..fest.js gzip	151 B	149 B	N/A
middleware.js gzip	47.4 kB	47.4 kB	N/A
edge-runtime..pack.js gzip	1.94 kB	1.94 kB	✓
Overall change	1.94 kB	1.94 kB	✓

Next Runtimes

	vercel/next.js canary	juliusmarminge/next.js allow-subrequests	Change
app-page-exp...dev.js gzip	166 kB	166 kB	✓
app-page-exp..prod.js gzip	95.1 kB	95.1 kB	✓
app-page-tur..prod.js gzip	96.9 kB	96.9 kB	✓
app-page-tur..prod.js gzip	91.4 kB	91.4 kB	✓
app-page.run...dev.js gzip	135 kB	135 kB	✓
app-page.run..prod.js gzip	90 kB	90 kB	✓
app-route-ex...dev.js gzip	22 kB	22 kB	✓
app-route-ex..prod.js gzip	14.8 kB	14.8 kB	✓
app-route-tu..prod.js gzip	14.8 kB	14.8 kB	✓
app-route-tu..prod.js gzip	14.6 kB	14.6 kB	✓
app-route.ru...dev.js gzip	21.7 kB	21.7 kB	✓
app-route.ru..prod.js gzip	14.6 kB	14.6 kB	✓
pages-api-tu..prod.js gzip	9.43 kB	9.43 kB	✓
pages-api.ru...dev.js gzip	9.7 kB	9.7 kB	✓
pages-api.ru..prod.js gzip	9.43 kB	9.43 kB	✓
pages-turbo...prod.js gzip	22 kB	22 kB	✓
pages.runtim...dev.js gzip	22.7 kB	22.7 kB	✓
pages.runtim..prod.js gzip	22 kB	22 kB	✓
server.runti..prod.js gzip	49.7 kB	49.7 kB	✓
Overall change	922 kB	922 kB	✓

Commit: 47e7430

juliusmarminge · 2024-02-05T23:13:19Z

packages/next/src/server/web/sandbox/sandbox.ts

+    let result: FetchEventResult | undefined = undefined
+    await requestStore.run({ headers }, async () => {
+      result = await edgeFunction({
+        request: {
+          ...params.request,
+          body:
+            cloned && requestToBodyStream(runtime.context, KUint8Array, cloned),
+        },
+      })
+      for (const headerName of FORBIDDEN_HEADERS) {
+        result.response.headers.delete(headerName)
+      }
    })
-    for (const headerName of FORBIDDEN_HEADERS) {
-      result.response.headers.delete(headerName)
-    }
+    if (!result) throw new Error('Edge function did not return a response')


this looks very sketchy - idk how far up the store.run call can be hoisted though or if there a better way to do this...

Hoisting to this level seems alright as it looks like we only need to access it in the same Node.js context.

ijjk · 2024-02-06T17:41:16Z

packages/next/src/server/web/sandbox/context.ts

+          const store = requestStore.getStore()
+          if (store) {
+            new Headers(init?.headers).forEach((value, key) => {
+              store.headers.set(key, value)
+            })
+          }


Why are we needing to augment the store from headers in a new Request(), shouldn't this only be done from the initial request event in the sandbox?

that does sound correct - should be able to remove this here then

confirmed it was unnecessary, removed it 👍🏼

ijjk

Changes and added test look good, thanks for the PR!

feat: allow overriding subrequest blocker

a116cbd

juliusmarminge requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners January 13, 2024 22:35

ijjk added the type: next label Jan 13, 2024

juliusmarminge mentioned this pull request Jan 20, 2024

try workaround to support next.js edge runtime pingdotgg/uploadthing#550

Closed

ijjk reviewed Feb 5, 2024

View reviewed changes

packages/next/src/server/web/sandbox/sandbox.ts Outdated Show resolved Hide resolved

juliusmarminge added 2 commits February 5, 2024 20:32

Merge branch 'canary' into allow-subrequests

4c91e02

max depth limit

5c75071

juliusmarminge changed the title ~~feat: allow overriding subrequest blocker~~ fix: allow some recursion for middleware subrequests Feb 5, 2024

ijjk added the CI approved Approve running CI for fork label Feb 5, 2024

Merge branch 'canary' into allow-subrequests

d5840a7

juliusmarminge added 2 commits February 5, 2024 23:53

store and forward headers from async local storage

60a54c4

add test

b180b21

ijjk added the area: tests label Feb 5, 2024

juliusmarminge commented Feb 5, 2024

View reviewed changes

skip deploy mode

b1701e3

ijjk reviewed Feb 6, 2024

View reviewed changes

juliusmarminge added 2 commits February 6, 2024 18:50

rm unnecessary

4542f83

Merge branch 'canary' into allow-subrequests

47e7430

ijjk approved these changes Feb 6, 2024

View reviewed changes

ijjk enabled auto-merge (squash) February 6, 2024 17:59

ijjk merged commit 6d07c00 into vercel:canary Feb 6, 2024
61 of 66 checks passed

github-actions bot added the locked label Feb 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow some recursion for middleware subrequests #60615

fix: allow some recursion for middleware subrequests #60615

juliusmarminge commented Jan 13, 2024 •

edited

ijjk commented Jan 13, 2024

ijjk commented Jan 13, 2024 •

edited

ijjk commented Feb 5, 2024 •

edited

juliusmarminge Feb 5, 2024

ijjk Feb 6, 2024

ijjk Feb 6, 2024

juliusmarminge Feb 6, 2024

juliusmarminge Feb 6, 2024

ijjk left a comment

fix: allow some recursion for middleware subrequests #60615

fix: allow some recursion for middleware subrequests #60615

Conversation

juliusmarminge commented Jan 13, 2024 • edited

ijjk commented Jan 13, 2024

ijjk commented Jan 13, 2024 • edited

ijjk commented Feb 5, 2024 • edited

Stats from current PR

juliusmarminge Feb 5, 2024

Choose a reason for hiding this comment

ijjk Feb 6, 2024

Choose a reason for hiding this comment

ijjk Feb 6, 2024

Choose a reason for hiding this comment

juliusmarminge Feb 6, 2024

Choose a reason for hiding this comment

juliusmarminge Feb 6, 2024

Choose a reason for hiding this comment

ijjk left a comment

Choose a reason for hiding this comment

juliusmarminge commented Jan 13, 2024 •

edited

ijjk commented Jan 13, 2024 •

edited

ijjk commented Feb 5, 2024 •

edited