[Cache Components] Discriminate static shell validation errors by type (old) #85645
Conversation
ijjk
added
created-by: Next.js team
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Failing test suitesCommit: c13139b | About building and testing Next.js
Expand output● output: standalone with getStaticProps › should work correctly with output standalone
Expand output● app-dir edge runtime root layout › should not emit metadata files into bad paths ● app-dir edge runtime root layout › should mark static contain metadata routes as edge functions
Expand output● app-dir edge SSR › should handle edge only routes ● app-dir edge SSR › should retrieve cookies in a server component in the edge runtime ● app-dir edge SSR › should treat process as object without polyfill in edge runtime ● app-dir edge SSR › should handle /index routes correctly ● app-dir edge SSR › should generate matchers correctly in middleware manifest
Expand output● Cache Components Fallback Validation › should warn about missing Suspense when accessing params if static params are partially known at build time ● Cache Components Fallback Validation › should warn about missing Suspense when accessing params if static params are entirely missing at build time
Expand output● output: standalone with app dir › should handle trace files correctly for route groups (nodejs only) ● output: standalone with app dir › should work correctly with output standalone
Expand output● app-dir - missing required html tags › should show error overlay ● app-dir - missing required html tags › should reload when you fix the error
Expand output● useReportWebVitals hook › should send web-vitals
Expand output● app-dir action useActionState › should support submitting form state with JS ● app-dir action useActionState › should support submitting form state without JS ● app-dir action useActionState › should support hydrating the app from progressively enhanced form request ● app-dir action useActionState › should send the action to the provided permalink with form state when JS disabled
Expand output● app-dir action handling › should output exportName and filename info in manifest ● app-dir action handling › should handle action correctly with middleware rewrite ● app-dir action handling › should handle basic actions correctly ● app-dir action handling › should report errors with bad inputs correctly ● app-dir action handling › should propagate errors from a ● app-dir action handling › should trigger an error boundary for action responses with an invalid content-type ● app-dir action handling › should support headers and cookies ● app-dir action handling › should support setting cookies when redirecting (with javascript) ● app-dir action handling › should support setting cookies when redirecting (no javascript) ● app-dir action handling › should push new route when redirecting ● app-dir action handling › should replace current route when redirecting with type set to replace ● app-dir action handling › should support headers in client imported actions ● app-dir action handling › should not log errors for non-action form POSTs ● app-dir action handling › should support setting cookies in route handlers with the correct overrides ● app-dir action handling › should support formData and redirect ● app-dir action handling › should support .bind ● app-dir action handling › should support chained .bind ● app-dir action handling › should support notFound (javascript disabled) ● app-dir action handling › should support notFound ● app-dir action handling › should support uploading files ● app-dir action handling › should support hoc auth wrappers ● app-dir action handling › should support importing actions in client components ● app-dir action handling › should support importing the same action module instance in both server and action layers ● app-dir action handling › should not block navigation events while a server action is in flight ● app-dir action handling › should not block router.back() while a server action is in flight ● app-dir action handling › should trigger a refresh for a server action that also dispatches a navigation event ● app-dir action handling › should support next/dynamic with ssr: false ● app-dir action handling › should support next/dynamic with ssr: false (edge) ● app-dir action handling › should only submit action once when resubmitting an action after navigation ● app-dir action handling › should handle actions executed in quick succession ● app-dir action handling › should reset the form state when the action redirects to a page that contains the same form ● app-dir action handling › should invalidate the client router cache if the redirect action triggers a revalidation ● app-dir action handling › should reset the form state when the action redirects to itself ● app-dir action handling › should be possible to catch network errors ● app-dir action handling › should be possible to catch regular errors ● app-dir action handling › should keep action instances identical ● app-dir action handling › should forward action request to a worker that contains the action handler (node) ● app-dir action handling › should forward action request to a worker that contains the action handler (edge) ● app-dir action handling › should not error when a forwarded action triggers a redirect (node) ● app-dir action handling › should not error when a forwarded action triggers a redirect (edge) ● app-dir action handling › should not expose action content in sourcemaps ● app-dir action handling › Edge SSR › should handle basic actions correctly ● app-dir action handling › Edge SSR › should return error response for hoc auth wrappers in edge runtime ● app-dir action handling › Edge SSR › should handle calls to redirect() with a relative URL in a single pass ● app-dir action handling › Edge SSR › should handle calls to redirect() with a absolute URL in a single pass ● app-dir action handling › Edge SSR › should handle calls to redirect() with external URLs ● app-dir action handling › Edge SSR › should allow cookie and header async storages ● app-dir action handling › Edge SSR › should handle unicode search params ● app-dir action handling › fetch actions › should handle a fetch action initiated from a static page ● app-dir action handling › fetch actions › should handle calls to redirect() with a relative URL in a single pass ● app-dir action handling › fetch actions › should handle calls to redirect() with a absolute URL in a single pass ● app-dir action handling › fetch actions › should handle calls to redirect() with external URLs ● app-dir action handling › fetch actions › should handle redirects to routes that provide an invalid RSC response ● app-dir action handling › fetch actions › should handle revalidatePath ● app-dir action handling › fetch actions › should handle revalidateTag ● app-dir action handling › fetch actions › should handle revalidateTag + redirect ● app-dir action handling › fetch actions › should store revalidation data in the prefetch cache ● app-dir action handling › fetch actions › should revalidate when cookies.set is called ● app-dir action handling › fetch actions › should invalidate client cache on other routes when cookies.set is called ● app-dir action handling › fetch actions › should revalidate when cookies.set is called in a client action ● app-dir action handling › fetch actions › should invalidate client cache when tag is revalidated ● app-dir action handling › fetch actions › should invalidate client cache when path is revalidated ● app-dir action handling › should work with interception routes ● app-dir action handling › encryption › should send encrypted values from the closed over closure ● app-dir action handling › encryption › should be able to resolve other server actions and client components ● app-dir action handling › redirects › redirects properly when route handler uses ● app-dir action handling › redirects › redirects properly when route handler uses ● app-dir action handling › redirects › displays searchParams correctly when redirecting with SearchParams ● app-dir action handling › redirects › merges cookies correctly when redirecting ● app-dir action handling › redirects › should not forward next-action header to a redirected RSC request ● app-dir action handling › redirects › redirects properly when route handler redirects with a 307 status code ● app-dir action handling › redirects › redirects properly when route handler redirects with a 308 status code ● app-dir action handling › server actions render client components › server component imported action › should support importing client components from actions ● app-dir action handling › server actions render client components › client component imported action › should support importing client components from actions ● app-dir action handling › caching disabled by default › should use no-store as default for server action ● app-dir action handling › caching disabled by default › should not override force-cache in server action ● app-dir action handling › caching disabled by default › should not override revalidate in server action ● app-dir action handling › request body decoding › should correctly decode multi-byte characters in the request body (node) ● app-dir action handling › request body decoding › should correctly decode multi-byte characters in the request body (edge) ● app-dir action handling › action discarding › should not trigger a refresh for a server action that gets discarded due to a navigation (without revalidation) ● app-dir action handling › action discarding › should trigger a refresh for a server action that gets discarded due to a navigation (with revalidation)
Expand output● Cache Components Dev Errors › should show a red box error on the SSR render ● Cache Components Dev Errors › should not show a red box error on client navigations ● Cache Components Dev Errors › should display error when component accessed data without suspense boundary
Expand output● app dir - basic › should have correct cache-control for SSR routes ● app dir - basic › should provide query for getStaticProps page correctly ● app dir - basic › should contain framework.json ● app dir - basic › outputs correct build-diagnostics.json ● app dir - basic › should work for catch-all edge page ● app dir - basic › should return normalized dynamic route params for catch-all edge page ● app dir - basic › should have correct searchParams and params (server) ● app dir - basic › should have correct searchParams and params (client) ● app dir - basic › should successfully detect app route during prefetch ● app dir - basic › should encode chunk path correctly ● app dir - basic › should match redirects in pages correctly $path ● app dir - basic › should match redirects in pages correctly $path ● app dir - basic › should match redirects in pages correctly $path ● app dir - basic › should match redirects in pages correctly $path ● app dir - basic › should match redirects in pages correctly $path ● app dir - basic › should match redirects in pages correctly $path ● app dir - basic › should not apply client router filter on shallow ● app dir - basic › should not share edge workers ● app dir - basic › should generate build traces correctly ● app dir - basic › should use text/x-component for flight ● app dir - basic › should use text/x-component for flight with edge runtime ● app dir - basic › should return the ● app dir - basic › should return the ● app dir - basic › should pass props from getServerSideProps in root layout ● app dir - basic › should serve from pages ● app dir - basic › should serve dynamic route from pages ● app dir - basic › should serve from public ● app dir - basic › should serve from app ● app dir - basic › should ensure the suffix is at the end of the stream ● app dir - basic › should serve /index as separate page ● app dir - basic › should serve polyfills for browsers that do not support modules ● app dir - basic › should handle css imports in next/dynamic correctly ● app dir - basic › should include layouts when no direct parent layout ● app dir - basic › should not include parent when not in parent directory with route in directory ● app dir - basic › should use new root layout when provided ● app dir - basic › should not create new root layout when nested (optional) ● app dir - basic › should include parent document when no direct parent layout ● app dir - basic › should not include parent when not in parent directory ● app dir - basic › should serve nested parent ● app dir - basic › should serve dynamic parameter ● app dir - basic › should serve page as a segment name correctly ● app dir - basic › should include document html and body ● app dir - basic › should not serve when layout is provided but no folder index ● app dir - basic › should match partial parameters ● app dir - basic › rewrites › should support rewrites on initial load ● app dir - basic › rewrites › should support rewrites on client-side navigation from pages to app with existing pages path ● app dir - basic › rewrites › should support rewrites on client-side navigation ● app dir - basic › should not rerender layout when navigating between routes in the same layout ● app dir - basic › should handle hash in initial url ● app dir - basic › › should hard push ● app dir - basic › › should hard replace ● app dir - basic › › should soft push ● app dir - basic › › should soft replace ● app dir - basic › › should be soft for back navigation ● app dir - basic › › should be soft for forward navigation ● app dir - basic › › should allow linking from app page to pages page ● app dir - basic › › should navigate to pages dynamic route from pages page if it overlaps with an app page ● app dir - basic › › should push to external url ● app dir - basic › › should replace to external url ● app dir - basic › server components › should not serve .server.js as a path ● app dir - basic › server components › should not serve .client.js as a path ● app dir - basic › server components › should serve shared component ● app dir - basic › server components › dynamic routes › should only pass params that apply to the layout ● app dir - basic › server components › catch-all routes › should handle optional segments ● app dir - basic › server components › catch-all routes › should handle optional segments root ● app dir - basic › server components › catch-all routes › should handle optional catch-all segments link ● app dir - basic › server components › catch-all routes › should handle required segments ● app dir - basic › server components › catch-all routes › should handle required segments root as not found ● app dir - basic › server components › catch-all routes › should handle catch-all segments link ● app dir - basic › server components › should serve client component › should serve server-side ● app dir - basic › server components › should serve client component › should serve client-side ● app dir - basic › server components › should include client component layout with server component route › should include it server-side ● app dir - basic › server components › should include client component layout with server component route › should include it client-side ● app dir - basic › server components › Loading › should render loading.js in initial html for slow page ● app dir - basic › server components › Loading › should render loading.js in browser for slow page ● app dir - basic › server components › Loading › should render loading.js in initial html for slow layout ● app dir - basic › server components › Loading › should render loading.js in browser for slow layout ● app dir - basic › server components › Loading › should render loading.js in initial html for slow layout and page ● app dir - basic › server components › Loading › should render loading.js in browser for slow layout and page ● app dir - basic › server components › middleware › should strip internal query parameters from requests to middleware for rewrite ● app dir - basic › server components › middleware › should strip internal query parameters from requests to middleware for redirect ● app dir - basic › server components › next/router › should support router.back and router.forward ● app dir - basic › server components › client components › should have consistent query and params handling ● app dir - basic › searchParams prop › client component › should have the correct search params ● app dir - basic › searchParams prop › client component › should have the correct search params on rewrite ● app dir - basic › searchParams prop › client component › should have the correct search params on middleware rewrite ● app dir - basic › searchParams prop › server component › should have the correct search params ● app dir - basic › searchParams prop › server component › should have the correct search params on rewrite ● app dir - basic › searchParams prop › server component › should have the correct search params on middleware rewrite ● app dir - basic › template component › should render the template that holds state in a client component and reset on navigation ● app dir - basic › template component › should render the template that is a server component and rerender on navigation ● app dir - basic › known bugs › should support React cache › server component ● app dir - basic › known bugs › should support React cache › server component client-navigation ● app dir - basic › known bugs › should support React cache › client component ● app dir - basic › known bugs › should support React cache › client component client-navigation ● app dir - basic › known bugs › should support React cache › middleware overriding headers ● app dir - basic › known bugs › should support React fetch instrumentation › server component ● app dir - basic › known bugs › should support React fetch instrumentation › server component client-navigation ● app dir - basic › known bugs › should support React fetch instrumentation › client component ● app dir - basic › known bugs › should support React fetch instrumentation › client component client-navigation ● app dir - basic › known bugs › should not share flight data between requests ● app dir - basic › known bugs › should handle router.refresh without resetting state ● app dir - basic › known bugs › should handle as on next/link ● app dir - basic › known bugs › should handle next/link back to initially loaded page ● app dir - basic › known bugs › should not do additional pushState when already on the page ● app dir - basic › next/script › should support next/script and render in correct order ● app dir - basic › next/script › should pass on extra props for beforeInteractive scripts with a src prop ● app dir - basic › next/script › should pass on extra props for beforeInteractive scripts without a src prop ● app dir - basic › next/script › should insert preload tags for beforeInteractive and afterInteractive scripts ● app dir - basic › next/script › should load stylesheets for next/scripts ● app dir - basic › next/script › should pass ● app dir - basic › next/script › should pass manual ● app dir - basic › next/script › should pass manual ● app dir - basic › next/script › should pass nonce when using next/font ● app dir - basic › data fetch with response over 16KB with chunked encoding › should load page when fetching a large amount of data ● app dir - basic › bootstrap scripts › should only bootstrap with one script, prinitializing the rest ● app dir - basic › bootstrap scripts › should successfully bootstrap even when using CSP ● app dir - basic › should run generate command correctly
Expand output● Cache Components Errors › Sync IO in console methods › Console Patching › does not warn about sync IO if console.log is patched to call new Date() internally
Expand output● app-dir-hmr › filesystem changes › should update server components after navigating to a page with a different runtime ● app-dir-hmr › filesystem changes › can navigate cleanly to a page that requires a change in the Webpack runtime
Expand output● app dir - metadata › basic › should support title and description ● app dir - metadata › basic › should support title template ● app dir - metadata › basic › should support stashed title in one layer of page and layout ● app dir - metadata › basic › should use parent layout title when no title is defined in page ● app dir - metadata › basic › should support stashed title in two layers of page and layout ● app dir - metadata › basic › should support other basic tags ● app dir - metadata › basic › should support other basic tags (edge) ● app dir - metadata › basic › should support apple related tags ● app dir - metadata › basic › should support socials related tags like facebook and pinterest ● app dir - metadata › basic › should support alternate tags ● app dir - metadata › basic › should relative canonical url ● app dir - metadata › basic › should not contain query in canonical url after client navigation ● app dir - metadata › basic › should support robots tags ● app dir - metadata › basic › should support verification tags ● app dir - metadata › basic › should support appLinks tags ● app dir - metadata › basic › should apply metadata when navigating client-side ● app dir - metadata › basic › should support generateMetadata dynamic props ● app dir - metadata › basic › should handle metadataBase for urls resolved as only URL type ● app dir - metadata › basic › should handle metadataBase as url string ● app dir - metadata › opengraph › should support opengraph tags ● app dir - metadata › opengraph › should support opengraph with article type ● app dir - metadata › opengraph › should pick up opengraph-image and twitter-image as static metadata files ● app dir - metadata › opengraph › should override file based images when opengraph-image and twitter-image specify images property ● app dir - metadata › opengraph › metadataBase should override fallback base for resolving OG images ● app dir - metadata › icons › should support basic object icons field ● app dir - metadata › icons › should support basic string icons field ● app dir - metadata › icons › should support basic complex descriptor icons field ● app dir - metadata › icons › should merge icons from layout if no static icons files are specified ● app dir - metadata › icons › should not hoist meta[itemProp] to head ● app dir - metadata › icons › should support root level of favicon.ico ● app dir - metadata › file based icons › should render icon and apple touch icon meta if their images are specified ● app dir - metadata › file based icons › should not render if image file is not specified ● app dir - metadata › twitter › should support twitter card summary_large_image when image present ● app dir - metadata › twitter › should render twitter card summary when image is not present ● app dir - metadata › twitter › should support default twitter player card ● app dir - metadata › twitter › should support default twitter app card ● app dir - metadata › static routes › should have /favicon.ico as route ● app dir - metadata › static routes › should have icons as route ● app dir - metadata › static routes › should support root dir robots.txt ● app dir - metadata › static routes › should support sitemap.xml under every routes ● app dir - metadata › static routes › should support static manifest.webmanifest ● app dir - metadata › static routes › should build favicon.ico as a custom route ● app dir - metadata › static optimization › should build static files into static route ● app dir - metadata › viewport › should support dynamic viewport export ● app dir - metadata › viewport › should skip initial-scale from viewport if it is set to undefined ● app dir - metadata › react cache › should have same title and page value on initial load ● app dir - metadata › react cache › should have same title and page value when navigating ● app dir - metadata › should not effect metadata images convention like files under pages directory ● app dir - metadata › regression: renders a large shell |
| { once: true } | ||
| ) | ||
|
|
||
| this.mayAbandon = true |
There was a problem hiding this comment.
nit: not sure if we need a separate property for this? afaict this.mayAbandon === !!this.abortSignal, so we can make it a getter instead
gnoff
force-pushed
the
discriminated-dynamic-errors
branch
from
b3792ff to
a0cf548
Compare
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| if (initialStageController.currentStage === RenderStage.Abandoned) { | ||
| // If we abandoned the render in the static stage, we won't proceed further. | ||
| return null | ||
| } |
There was a problem hiding this comment.
seems like this can't currently happen, abandonRender() is only called in runtime/dynamic stages, because we decided that we always want to go into the runtime stage to warm runtime caches. was this meant to check for sync IO aborts or something?
There was a problem hiding this comment.
ah, my mistake -- in the initial stage, sync IO will cause the render to be abandoned, so it can happen here too. we should probably add a comment about that, and note that abandonRender() will still advance us to the runtime stage, so technically we've already proceeded to further stages, just not along the regular path.
gnoff
force-pushed
the
discriminated-dynamic-errors
branch
from
a0cf548 to
0ddd2f5
Compare
unstubbable
force-pushed
the
discriminated-dynamic-errors
branch
from
0ddd2f5 to
49feecf
Compare
gnoff
force-pushed
the
discriminated-dynamic-errors
branch
from
49feecf to
df76b85
Compare
Prior to this change any "hole" in a prerender that would block the shell was considered an error and you would be presented with a very generic message explaining all the different ways you could have failed this validation check. With this change we use a new technique to validate the static shell which can now tell the difference between waiting on uncached data or runtime data. It also improves the heuristics around generateMetadata and generateViewport errors. I added new error pages for runtime sync IO and ensure we only validate sync IO after runtime data if the page will be validating runtime prefetches. I restored the validation on HMR update so you can get feedback after saving a new file.
in theory this shouldn't be necessary but currently we only track awaits and just rendering the promise as is doesn't cause there to be and IO stack line to generate a codeframe off of
This ensures that any I/O awaited during the dynamic stage (i.e. after other uncached I/O resolved) does not contaminate the owner stacks used for error reporting.
Removing the old implementation of `spawnDynamicValidationInDev` seems to make the stray error messages go away.
unstubbable
force-pushed
the
discriminated-dynamic-errors
branch
from
b3c47a4 to
c13139b
Compare
| one: () => A, | ||
| two: (a: A) => B, | ||
| three: (b: B) => C | Promise<C> | ||
| three: (b: B) => C |
There was a problem hiding this comment.
The pipelineInSequentialTasks function signature was narrowed to disallow returning promises, but it's actually called with async functions that do return promises, creating a type safety regression.
View Details
📝 Patch Details
diff --git a/packages/next/src/server/app-render/app-render-render-utils.ts b/packages/next/src/server/app-render/app-render-render-utils.ts
index 2645ca3343..58b236d1d0 100644
--- a/packages/next/src/server/app-render/app-render-render-utils.ts
+++ b/packages/next/src/server/app-render/app-render-render-utils.ts
@@ -38,7 +38,7 @@ export function scheduleInSequentialTasks<R>(
export function pipelineInSequentialTasks<A, B, C>(
one: () => A,
two: (a: A) => B,
- three: (b: B) => C
+ three: (b: B) => C | Promise<C>
): Promise<C> {
if (process.env.NEXT_RUNTIME === 'edge') {
throw new InvariantError(
@@ -71,7 +71,7 @@ export function pipelineInSequentialTasks<A, B, C>(
}
}, 0)
- let threeResult: C
+ let threeResult: C | Promise<C>
const threeId = setTimeout(() => {
// if `two` threw, then this timeout would've been cleared,
// so if we got here, we're guaranteed to have a value.
@@ -85,7 +85,7 @@ export function pipelineInSequentialTasks<A, B, C>(
// We wait a task before resolving/rejecting
const fourId = setTimeout(() => {
- resolve(threeResult)
+ resolve(threeResult as any)
}, 0)
})
}
Analysis
Type Safety Regression in pipelineInSequentialTasks Function
What fails: The pipelineInSequentialTasks function signature was narrowed from three: (b: B) => C | Promise<C> to three: (b: B) => C, removing Promise support. However, the function is called with async functions at lines 3253 and 3416 in packages/next/src/server/app-render/app-render.tsx, which always return Promise<...>. This creates a type mismatch where the calling code violates the new type signature.
How to reproduce:
cd packages/next
pnpm exec tsc --noEmitCheck that files calling pipelineInSequentialTasks with async third parameters now type-check correctly.
Result before fix: The function signature only accepts (b: B) => C for the third parameter, preventing async functions even though the codebase uses them.
Result after fix: The function signature now accepts (b: B) => C | Promise<C>, properly reflecting the actual usage patterns where async functions are passed as the third parameter. The implementation correctly handles both synchronous and asynchronous returns through Promise.resolve() semantics.
Why this matters: This fixes a type safety regression where a recent signature change removed Promise support without updating the calling code, creating a silent type mismatch that could hide legitimate errors from TypeScript's type checker.
|
Closing in favor of #85747 |
lubieowoce
changed the title
4 participants
Prior to this change any "hole" in a prerender that would block the shell was considered an error and you would be presented with a very generic message explaining all the different ways you could have failed this validation check.
With this change we use a new technique to validate the static shell which can now tell the difference between waiting on uncached data or runtime data. It also improves the heuristics around generateMetadata and generateViewport errors.
I added new error pages for runtime sync IO and ensure we only validate sync IO after runtime data if the page will be validating runtime prefetches.
I restored the validation on HMR update so you can get feedback after saving a new file.