[ppr] Don't mark RSC requests as /_next/data requests (#66249)

Old logic from the pages router was previously being hit during
development. This was more apparent when PPR was enabled as it was
mixing dynamic + static rendering in development which propagated to
errors. This change ensures that requests that are made with `RSC: 1`
are not marked as `/_next/data` URL's, and don't use the same logic
paths.

Previously it was a bit confusing because we used the variable
`isDataReq` in a few places that made it hard to tell what it was
referring to. In this case, the `isDataReq` is actually only used by the
pages router. This renames the `isDataReq` to `isNextDataRequest` to
make it clearer, as well as refactors to ensure that it's not used in
the paths for app routes.

Also to better represent the rendering modes the `supportsDynamicHTML`
variable was renamed to `supportsDynamicResponse`.

Fixes #66241

lint-staged.config.js

@@ -30,7 +30,12 @@ module.exports = {
 
     return [
       `prettier --with-node-modules --ignore-path .prettierignore --write ${escapedFileNames}`,
-      `eslint --no-ignore --max-warnings=0 --fix ${escape(eslintFileNames.filter((filename) => filename !== null)).join(' ')}`,
+      `eslint --no-ignore --max-warnings=0 --fix ${eslintFileNames
+        .filter((filename) => filename !== null)
+        .map((filename) => {
+          return `"${filename}"`
+        })
+        .join(' ')}`,
       `git add ${escapedFileNames}`,
     ]
   },

packages/next/src/build/utils.ts

@@ -1389,7 +1389,7 @@ export async function buildAppStaticPaths({
       renderOpts: {
         originalPathname: page,
         incrementalCache,
-        supportsDynamicHTML: true,
+        supportsDynamicResponse: true,
         isRevalidate: false,
         experimental: {
           after: false,

packages/next/src/build/webpack/loaders/next-edge-ssr-loader/render.ts

@@ -93,7 +93,7 @@ export function getRender({
       extendRenderOpts: {
         buildId,
         runtime: SERVER_RUNTIME.experimentalEdge,
-        supportsDynamicHTML: true,
+        supportsDynamicResponse: true,
         disableOptimizedLoading: true,
         serverActionsManifest,
         serverActions,

packages/next/src/export/index.ts

@@ -398,7 +398,7 @@ export async function exportAppImpl(
     domainLocales: i18n?.domains,
     disableOptimizedLoading: nextConfig.experimental.disableOptimizedLoading,
     // Exported pages do not currently support dynamic HTML.
-    supportsDynamicHTML: false,
+    supportsDynamicResponse: false,
     crossOrigin: nextConfig.crossOrigin,
     optimizeCss: nextConfig.experimental.optimizeCss,
     nextConfigOutput: nextConfig.output,

packages/next/src/export/routes/app-route.ts

@@ -75,7 +75,7 @@ export async function exportAppRoute(
       experimental: experimental,
       originalPathname: page,
       nextExport: true,
-      supportsDynamicHTML: false,
+      supportsDynamicResponse: false,
       incrementalCache,
       waitUntil: undefined,
       onClose: undefined,

packages/next/src/export/worker.ts

@@ -269,7 +269,7 @@ async function exportPageImpl(
       disableOptimizedLoading,
       fontManifest: optimizeFonts ? requireFontManifest(distDir) : undefined,
       locale,
-      supportsDynamicHTML: false,
+      supportsDynamicResponse: false,
       originalPathname: page,
       experimental: {
         ...input.renderOpts.experimental,

packages/next/src/server/app-render/app-render.tsx

@@ -643,7 +643,7 @@ async function renderToHTMLOrFlightImpl(
     ComponentMod,
     dev,
     nextFontManifest,
-    supportsDynamicHTML,
+    supportsDynamicResponse,
     serverActions,
     appDirDevErrorLogger,
     assetPrefix = '',
@@ -781,7 +781,7 @@ async function renderToHTMLOrFlightImpl(
    * These rules help ensure that other existing features like request caching,
    * coalescing, and ISR continue working as intended.
    */
-  const generateStaticHTML = supportsDynamicHTML !== true
+  const generateStaticHTML = supportsDynamicResponse !== true
 
   // Pull out the hooks/references from the component.
   const { tree: loaderTree, taintObjectReference } = ComponentMod

packages/next/src/server/app-render/types.ts

@@ -129,7 +129,7 @@ export interface RenderOptsPartial {
   basePath: string
   trailingSlash: boolean
   clientReferenceManifest?: DeepReadonly<ClientReferenceManifest>
-  supportsDynamicHTML: boolean
+  supportsDynamicResponse: boolean
   runtime?: ServerRuntime
   serverComponents?: boolean
   enableTainting?: boolean

packages/next/src/server/async-storage/static-generation-async-storage-wrapper.ts

@@ -41,7 +41,7 @@ export type StaticGenerationContext = {
     // mirrored.
     RenderOptsPartial,
     | 'originalPathname'
-    | 'supportsDynamicHTML'
+    | 'supportsDynamicResponse'
     | 'isRevalidate'
     | 'nextExport'
     | 'isDraftMode'
@@ -77,7 +77,7 @@ export const StaticGenerationAsyncStorageWrapper: AsyncStorageWrapper<
      * coalescing, and ISR continue working as intended.
      */
     const isStaticGeneration =
-      !renderOpts.supportsDynamicHTML &&
+      !renderOpts.supportsDynamicResponse &&
       !renderOpts.isDraftMode &&
       !renderOpts.isServerAction
 

packages/next/src/server/base-server.ts

@@ -527,7 +527,7 @@ export default abstract class Server<
     }
 
     this.renderOpts = {
-      supportsDynamicHTML: true,
+      supportsDynamicResponse: true,
       trailingSlash: this.nextConfig.trailingSlash,
       deploymentId: this.nextConfig.deploymentId,
       strictNextHead: this.nextConfig.experimental.strictNextHead ?? true,
@@ -649,10 +649,6 @@ export default abstract class Server<
       return false
     }
 
-    // If we're here, this is a data request, as it didn't return and it matched
-    // either a RSC or a prefetch RSC request.
-    parsedUrl.query.__nextDataReq = '1'
-
     if (req.url) {
       const parsed = parseUrl(req.url)
       parsed.pathname = parsedUrl.pathname
@@ -1601,7 +1597,7 @@ export default abstract class Server<
       ...partialContext,
       renderOpts: {
         ...this.renderOpts,
-        supportsDynamicHTML: !isBotRequest,
+        supportsDynamicResponse: !isBotRequest,
         isBot: !!isBotRequest,
       },
     }
@@ -1647,7 +1643,7 @@ export default abstract class Server<
       ...partialContext,
       renderOpts: {
         ...this.renderOpts,
-        supportsDynamicHTML: false,
+        supportsDynamicResponse: false,
       },
     }
     const payload = await fn(ctx)
@@ -1946,7 +1942,7 @@ export default abstract class Server<
     }
 
     // Toggle whether or not this is a Data request
-    let isDataReq =
+    const isNextDataRequest =
       !!(
         query.__nextDataReq ||
         (req.headers['x-nextjs-data'] &&
@@ -2056,7 +2052,7 @@ export default abstract class Server<
       isRoutePPREnabled && isRSCRequest && !isPrefetchRSCRequest
 
     // we need to ensure the status code if /404 is visited directly
-    if (is404Page && !isDataReq && !isRSCRequest) {
+    if (is404Page && !isNextDataRequest && !isRSCRequest) {
       res.statusCode = 404
     }
 
@@ -2097,7 +2093,7 @@ export default abstract class Server<
     // the query object. This ensures it won't be found by the `in` operator.
     if ('amp' in query && !query.amp) delete query.amp
 
-    if (opts.supportsDynamicHTML === true) {
+    if (opts.supportsDynamicResponse === true) {
       const isBotRequest = isBot(req.headers['user-agent'] || '')
       const isSupportedDocument =
         typeof components.Document?.getInitialProps !== 'function' ||
@@ -2109,19 +2105,14 @@ export default abstract class Server<
       // TODO-APP: should the first render for a dynamic app path
       // be static so we can collect revalidate and populate the
       // cache if there are no dynamic data requirements
-      opts.supportsDynamicHTML =
+      opts.supportsDynamicResponse =
         !isSSG && !isBotRequest && !query.amp && isSupportedDocument
       opts.isBot = isBotRequest
     }
 
     // In development, we always want to generate dynamic HTML.
-    if (
-      !isDataReq &&
-      isAppPath &&
-      opts.dev &&
-      opts.supportsDynamicHTML === false
-    ) {
-      opts.supportsDynamicHTML = true
+    if (!isNextDataRequest && isAppPath && opts.dev) {
+      opts.supportsDynamicResponse = true
     }
 
     const defaultLocale = isSSG
@@ -2144,27 +2135,20 @@ export default abstract class Server<
       }
     }
 
-    if (isAppPath) {
-      if (!this.renderOpts.dev && !isPreviewMode && isSSG && isRSCRequest) {
-        // If this is an RSC request but we aren't in minimal mode, then we mark
-        // that this is a data request so that we can generate the flight data
-        // only.
-        if (!this.minimalMode) {
-          isDataReq = true
-        }
-
-        // If this is a dynamic RSC request, ensure that we don't purge the
-        // flight headers to ensure that we will only produce the RSC response.
-        // We only need to do this in non-edge environments (as edge doesn't
-        // support static generation).
-        if (
-          !isDynamicRSCRequest &&
-          (!isEdgeRuntime(opts.runtime) ||
-            (this.serverOptions as any).webServerConfig)
-        ) {
-          stripFlightHeaders(req.headers)
-        }
-      }
+    // If this is a request for an app path that should be statically generated
+    // and we aren't in the edge runtime, strip the flight headers so it will
+    // generate the static response.
+    if (
+      isAppPath &&
+      !opts.dev &&
+      !isPreviewMode &&
+      isSSG &&
+      isRSCRequest &&
+      !isDynamicRSCRequest &&
+      (!isEdgeRuntime(opts.runtime) ||
+        (this.serverOptions as any).webServerConfig)
+    ) {
+      stripFlightHeaders(req.headers)
     }
 
     let isOnDemandRevalidate = false
@@ -2215,7 +2199,7 @@ export default abstract class Server<
 
     // remove /_next/data prefix from urlPathname so it matches
     // for direct page visit and /_next/data visit
-    if (isDataReq) {
+    if (isNextDataRequest) {
       resolvedUrlPathname = this.stripNextDataPath(resolvedUrlPathname)
       urlPathname = this.stripNextDataPath(urlPathname)
     }
@@ -2224,7 +2208,7 @@ export default abstract class Server<
     if (
       !isPreviewMode &&
       isSSG &&
-      !opts.supportsDynamicHTML &&
+      !opts.supportsDynamicResponse &&
       !isServerAction &&
       !minimalPostponed &&
       !isDynamicRSCRequest
@@ -2300,10 +2284,10 @@ export default abstract class Server<
 
     const doRender: Renderer = async ({ postponed }) => {
       // In development, we always want to generate dynamic HTML.
-      let supportsDynamicHTML: boolean =
-        // If this isn't a data request and we're not in development, then we
-        // support dynamic HTML.
-        (!isDataReq && opts.dev === true) ||
+      let supportsDynamicResponse: boolean =
+        // If we're in development, we always support dynamic HTML, unless it's
+        // a data request, in which case we only produce static HTML.
+        (!isNextDataRequest && opts.dev === true) ||
         // If this is not SSG or does not have static paths, then it supports
         // dynamic HTML.
         (!isSSG && !hasStaticPaths) ||
@@ -2346,7 +2330,7 @@ export default abstract class Server<
               serverActions: this.nextConfig.experimental.serverActions,
             }
           : {}),
-        isDataReq,
+        isNextDataRequest,
         resolvedUrl,
         locale,
         locales,
@@ -2367,7 +2351,7 @@ export default abstract class Server<
           ...opts.experimental,
           isRoutePPREnabled,
         },
-        supportsDynamicHTML,
+        supportsDynamicResponse,
         isOnDemandRevalidate,
         isDraftMode: isPreviewMode,
         isServerAction,
@@ -2377,9 +2361,9 @@ export default abstract class Server<
       }
 
       if (isDebugPPRSkeleton) {
-        supportsDynamicHTML = false
+        supportsDynamicResponse = false
         renderOpts.nextExport = true
-        renderOpts.supportsDynamicHTML = false
+        renderOpts.supportsDynamicResponse = false
         renderOpts.isStaticGeneration = true
         renderOpts.isRevalidate = true
         renderOpts.isDebugPPRSkeleton = true
@@ -2411,7 +2395,7 @@ export default abstract class Server<
                 after: renderOpts.experimental.after,
               },
               originalPathname: components.ComponentMod.originalPathname,
-              supportsDynamicHTML,
+              supportsDynamicResponse,
               incrementalCache,
               isRevalidate: isSSG,
               waitUntil: this.getWaitUntil(),
@@ -2725,7 +2709,7 @@ export default abstract class Server<
           throw new NoFallbackError()
         }
 
-        if (!isDataReq) {
+        if (!isNextDataRequest) {
           // Production already emitted the fallback as static HTML.
           if (isProduction) {
             const html = await this.getFallback(
@@ -2859,11 +2843,11 @@ export default abstract class Server<
       revalidate = 0
     } else if (
       typeof cacheEntry.revalidate !== 'undefined' &&
-      (!this.renderOpts.dev || (hasServerProps && !isDataReq))
+      (!this.renderOpts.dev || (hasServerProps && !isNextDataRequest))
     ) {
       // If this is a preview mode request, we shouldn't cache it. We also don't
       // cache 404 pages.
-      if (isPreviewMode || (is404Page && !isDataReq)) {
+      if (isPreviewMode || (is404Page && !isNextDataRequest)) {
         revalidate = 0
       }
 
@@ -2917,7 +2901,7 @@ export default abstract class Server<
           })
         )
       }
-      if (isDataReq) {
+      if (isNextDataRequest) {
         res.statusCode = 404
         res.body('{"notFound":true}').send()
         return null
@@ -2940,7 +2924,7 @@ export default abstract class Server<
         )
       }
 
-      if (isDataReq) {
+      if (isNextDataRequest) {
         return {
           type: 'json',
           body: RenderResult.fromStatic(
@@ -3015,7 +2999,7 @@ export default abstract class Server<
       // If the request is a data request, then we shouldn't set the status code
       // from the response because it should always be 200. This should be gated
       // behind the experimental PPR flag.
-      if (cachedData.status && (!isDataReq || !isRoutePPREnabled)) {
+      if (cachedData.status && (!isRSCRequest || !isRoutePPREnabled)) {
         res.statusCode = cachedData.status
       }
 
@@ -3028,13 +3012,9 @@ export default abstract class Server<
       // as preview mode is a dynamic request (bypasses cache) and doesn't
       // generate both HTML and payloads in the same request so continue to just
       // return the generated payload
-      if (isDataReq && !isPreviewMode) {
+      if (isRSCRequest && !isPreviewMode) {
         // If this is a dynamic RSC request, then stream the response.
-        if (isDynamicRSCRequest) {
-          if (cachedData.pageData) {
-            throw new Error('Invariant: Expected pageData to be undefined')
-          }
-
+        if (typeof cachedData.pageData !== 'string') {
           if (cachedData.postponed) {
             throw new Error('Invariant: Expected postponed to be undefined')
           }
@@ -3047,16 +3027,10 @@ export default abstract class Server<
             // distinguishing between `force-static` and pages that have no
             // postponed state.
             // TODO: distinguish `force-static` from pages with no postponed state (static)
-            revalidate: 0,
+            revalidate: isDynamicRSCRequest ? 0 : cacheEntry.revalidate,
           }
         }
 
-        if (typeof cachedData.pageData !== 'string') {
-          throw new Error(
-            `Invariant: expected pageData to be a string, got ${typeof cachedData.pageData}`
-          )
-        }
-
         // As this isn't a prefetch request, we should serve the static flight
         // data.
         return {
@@ -3126,7 +3100,7 @@ export default abstract class Server<
         // to the client on the same request.
         revalidate: 0,
       }
-    } else if (isDataReq) {
+    } else if (isNextDataRequest) {
       return {
         type: 'json',
         body: RenderResult.fromStatic(JSON.stringify(cachedData.pageData)),

packages/next/src/server/next-server.ts

@@ -1856,7 +1856,7 @@ export default class NextNodeServer extends BaseServer<
     }
 
     // For edge to "fetch" we must always provide an absolute URL
-    const isDataReq = !!query.__nextDataReq
+    const isNextDataRequest = !!query.__nextDataReq
     const initialUrl = new URL(
       getRequestMeta(params.req, 'initURL') || '/',
       'http://n'
@@ -1867,7 +1867,7 @@ export default class NextNodeServer extends BaseServer<
       ...params.params,
     }).toString()
 
-    if (isDataReq) {
+    if (isNextDataRequest) {
       params.req.headers['x-nextjs-data'] = '1'
     }
     initialUrl.search = queryString

packages/next/src/server/render.tsx

@@ -246,7 +246,7 @@ export type RenderOptsPartial = {
   ampValidator?: (html: string, pathname: string) => Promise<void>
   ampSkipValidation?: boolean
   ampOptimizerConfig?: { [key: string]: any }
-  isDataReq?: boolean
+  isNextDataRequest?: boolean
   params?: ParsedUrlQuery
   previewProps: __ApiPreviewProps | undefined
   basePath: string
@@ -268,7 +268,7 @@ export type RenderOptsPartial = {
   defaultLocale?: string
   domainLocales?: DomainLocale[]
   disableOptimizedLoading?: boolean
-  supportsDynamicHTML: boolean
+  supportsDynamicResponse: boolean
   isBot?: boolean
   runtime?: ServerRuntime
   serverComponents?: boolean
@@ -449,7 +449,7 @@ export async function renderToHTMLImpl(
     getStaticProps,
     getStaticPaths,
     getServerSideProps,
-    isDataReq,
+    isNextDataRequest,
     params,
     previewProps,
     basePath,
@@ -1170,7 +1170,7 @@ export async function renderToHTMLImpl(
 
   // Avoid rendering page un-necessarily for getServerSideProps data request
   // and getServerSideProps/getStaticProps redirects
-  if ((isDataReq && !isSSG) || metadata.isRedirect) {
+  if ((isNextDataRequest && !isSSG) || metadata.isRedirect) {
     return new RenderResult(JSON.stringify(props), {
       metadata,
     })

packages/next/src/server/request-meta.ts

@@ -189,6 +189,11 @@ type NextQueryMetadata = {
 
   __nextSsgPath?: string
   _nextBubbleNoFallback?: '1'
+
+  /**
+   * When set to `1`, the request is for the `/_next/data` route using the pages
+   * router.
+   */
   __nextDataReq?: '1'
   __nextCustomErrorRender?: '1'
   [NEXT_RSC_UNION_QUERY]?: string

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

@@ -125,9 +125,9 @@ export async function adapter(
   const buildId = requestUrl.buildId
   requestUrl.buildId = ''
 
-  const isDataReq = params.request.headers['x-nextjs-data']
+  const isNextDataRequest = params.request.headers['x-nextjs-data']
 
-  if (isDataReq && requestUrl.pathname === '/index') {
+  if (isNextDataRequest && requestUrl.pathname === '/index') {
     requestUrl.pathname = '/'
   }
 
@@ -169,7 +169,7 @@ export async function adapter(
    * need to know about this property neither use it. We add it for testing
    * purposes.
    */
-  if (isDataReq) {
+  if (isNextDataRequest) {
     Object.defineProperty(request, '__isData', {
       enumerable: false,
       value: true,
@@ -323,7 +323,7 @@ export async function adapter(
     )
 
     if (
-      isDataReq &&
+      isNextDataRequest &&
       // if the rewrite is external and external rewrite
       // resolving config is enabled don't add this header
       // so the upstream app can set it instead
@@ -367,7 +367,7 @@ export async function adapter(
      * it may end up with CORS error. Instead we map to an internal header so
      * the client knows the destination.
      */
-    if (isDataReq) {
+    if (isNextDataRequest) {
       response.headers.delete('Location')
       response.headers.set(
         'x-nextjs-redirect',

packages/next/src/server/web/edge-route-module-wrapper.ts

@@ -115,7 +115,7 @@ export class EdgeRouteModuleWrapper {
         notFoundRoutes: [],
       },
       renderOpts: {
-        supportsDynamicHTML: true,
+        supportsDynamicResponse: true,
         waitUntil,
         onClose: closeController
           ? closeController.onClose.bind(closeController)

test/e2e/app-dir/ppr-navigations/simple/app/[locale]/about/page.jsx

@@ -0,0 +1,5 @@
+import { TestPage } from '../../../components/page'
+
+export default function Page({ params: { locale } }) {
+  return <TestPage pathname={`/${locale}/about`} />
+}

test/e2e/app-dir/ppr-navigations/simple/app/[locale]/layout.jsx

@@ -0,0 +1,13 @@
+import { locales } from '../../components/page'
+
+export async function generateStaticParams() {
+  return locales.map((locale) => ({ locale }))
+}
+
+export default function Layout({ children, params: { locale } }) {
+  return (
+    <html lang={locale}>
+      <body>{children}</body>
+    </html>
+  )
+}

test/e2e/app-dir/ppr-navigations/simple/app/[locale]/page.jsx

@@ -0,0 +1,5 @@
+import { TestPage } from '../../components/page'
+
+export default function Page({ params: { locale } }) {
+  return <TestPage pathname={`/${locale}`} />
+}

test/e2e/app-dir/ppr-navigations/simple/app/layout.jsx

@@ -0,0 +1,3 @@
+export default ({ children }) => {
+  return children
+}

test/e2e/app-dir/ppr-navigations/simple/app/page.jsx

@@ -0,0 +1,7 @@
+import { redirect } from 'next/navigation'
+import { locales } from '../components/page'
+
+export default () => {
+  // Redirect to the default locale
+  return redirect(`/${locales[0]}`)
+}

test/e2e/app-dir/ppr-navigations/simple/components/page.jsx

@@ -0,0 +1,40 @@
+import { unstable_noStore } from 'next/cache'
+import Link from 'next/link'
+import { Suspense } from 'react'
+
+export const locales = ['en', 'fr']
+
+export const links = [
+  { href: '/', text: 'Home' },
+  ...locales
+    .map((locale) => {
+      return [
+        { href: `/${locale}`, text: locale },
+        { href: `/${locale}/about`, text: `${locale} - About` },
+      ]
+    })
+    .flat(),
+]
+
+function Dynamic() {
+  unstable_noStore()
+  return <div id="dynamic">Dynamic</div>
+}
+
+export function TestPage({ pathname }) {
+  return (
+    <div>
+      <ul>
+        {links.map(({ href, text }) => (
+          <li key={href}>
+            <Link href={href}>{text}</Link>
+          </li>
+        ))}
+      </ul>
+      <code data-value={pathname}>{pathname}</code>
+      <Suspense fallback={<div>Loading...</div>}>
+        <Dynamic />
+      </Suspense>
+    </div>
+  )
+}

test/e2e/app-dir/ppr-navigations/simple/next.config.js

@@ -0,0 +1,5 @@
+module.exports = {
+  experimental: {
+    ppr: true,
+  },
+}

test/e2e/app-dir/ppr-navigations/simple/simple.test.ts

@@ -0,0 +1,31 @@
+import { nextTestSetup } from 'e2e-utils'
+import { links, locales } from './components/page'
+
+describe('ppr-navigations simple', () => {
+  const { next } = nextTestSetup({
+    files: __dirname,
+  })
+
+  it('can navigate between all the links and back', async () => {
+    const browser = await next.browser('/')
+
+    try {
+      for (const { href } of links) {
+        // Find the link element for the href and click it.
+        await browser.elementByCss(`a[href="${href}"]`).click()
+
+        // Wait for that page to load.
+        if (href === '/') {
+          // The root page redirects to the first locale.
+          await browser.waitForElementByCss(`[data-value="/${locales[0]}"]`)
+        } else {
+          await browser.waitForElementByCss(`[data-value="${href}"]`)
+        }
+
+        await browser.elementByCss('#dynamic')
+      }
+    } finally {
+      await browser.close()
+    }
+  })
+})