From 4337bc264c58b065c1c42b2f5fdd3a3391dd3f82 Mon Sep 17 00:00:00 2001
From: Josh Story <story@hey.com>
Date: Thu, 26 Sep 2024 13:52:00 -0700
Subject: [PATCH] fallback shell should not error when dynamic due to params
 access even with dynamic = "error" (#70534)

When producing a fallback shell params is dynamic. Normally anything
dynamic shoudl be a build error when `export const dynamic = "error"` is
used. however for fallback shells we'll never have fully static shells,
nor should we since the whole point is to produce a PPR shell that
server a wide range of paths. In the refactor for async dynamic APIs I
introduced a bug where fallback param dynamic also errored if `export
const dynamic = "error"` was used. This change corrects this behavior
and adds a corresponding test
---
 packages/next/src/server/request/params.ts    | 32 +++++++++----------
 .../fallback/dynamic/error/[slug]/layout.jsx  | 10 ++++++
 .../fallback/dynamic/error/[slug]/page.jsx    | 14 ++++++++
 test/e2e/app-dir/ppr-full/ppr-full.test.ts    | 12 +++++++
 4 files changed, 51 insertions(+), 17 deletions(-)
 create mode 100644 test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/layout.jsx
 create mode 100644 test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/page.jsx

diff --git a/packages/next/src/server/request/params.ts b/packages/next/src/server/request/params.ts
index 1882806865498..1cdf6be2a101f 100644
--- a/packages/next/src/server/request/params.ts
+++ b/packages/next/src/server/request/params.ts
@@ -14,11 +14,7 @@ import {
   type PrerenderStore,
 } from '../app-render/prerender-async-storage.external'
 import { InvariantError } from '../../shared/lib/invariant-error'
-import {
-  makeResolvedReactPromise,
-  describeStringPropertyAccess,
-  throwWithStaticGenerationBailoutErrorWithDynamicError,
-} from './utils'
+import { makeResolvedReactPromise, describeStringPropertyAccess } from './utils'
 import { makeHangingPromise } from '../dynamic-rendering-utils'
 
 export type Params = Record<string, string | Array<string> | undefined>
@@ -259,12 +255,13 @@ function makeErroringExoticParams(
           Object.defineProperty(augmentedUnderlying, prop, {
             get() {
               const expression = describeStringPropertyAccess('params', prop)
-              if (staticGenerationStore.dynamicShouldError) {
-                throwWithStaticGenerationBailoutErrorWithDynamicError(
-                  staticGenerationStore.route,
-                  expression
-                )
-              } else if (prerenderStore) {
+              // In most dynamic APIs we also throw if `dynamic = "error"` however
+              // for params is only dynamic when we're generating a fallback shell
+              // and even when `dynamic = "error"` we still support generating dynamic
+              // fallback shells
+              // TODO remove this comment when dynamicIO is the default since there
+              // will be no `dynamic = "error"`
+              if (prerenderStore) {
                 postponeWithTracking(
                   staticGenerationStore.route,
                   expression,
@@ -282,12 +279,13 @@ function makeErroringExoticParams(
           Object.defineProperty(promise, prop, {
             get() {
               const expression = describeStringPropertyAccess('params', prop)
-              if (staticGenerationStore.dynamicShouldError) {
-                throwWithStaticGenerationBailoutErrorWithDynamicError(
-                  staticGenerationStore.route,
-                  expression
-                )
-              } else if (prerenderStore) {
+              // In most dynamic APIs we also throw if `dynamic = "error"` however
+              // for params is only dynamic when we're generating a fallback shell
+              // and even when `dynamic = "error"` we still support generating dynamic
+              // fallback shells
+              // TODO remove this comment when dynamicIO is the default since there
+              // will be no `dynamic = "error"`
+              if (prerenderStore) {
                 postponeWithTracking(
                   staticGenerationStore.route,
                   expression,
diff --git a/test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/layout.jsx b/test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/layout.jsx
new file mode 100644
index 0000000000000..e9d386641a47a
--- /dev/null
+++ b/test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/layout.jsx
@@ -0,0 +1,10 @@
+export const dynamic = 'error'
+
+export default function Layout({ children }) {
+  return (
+    <>
+      <div data-layout={Math.random().toString(16).slice(2)} />
+      {children}
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/page.jsx b/test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/page.jsx
new file mode 100644
index 0000000000000..592773a9a0bd5
--- /dev/null
+++ b/test/e2e/app-dir/ppr-full/app/fallback/dynamic/error/[slug]/page.jsx
@@ -0,0 +1,14 @@
+import { setTimeout } from 'timers/promises'
+
+export default async function Page({ params }) {
+  await setTimeout(1000)
+
+  const { slug } = params
+
+  return (
+    <div>
+      <div data-slug={slug}>{slug}</div>
+      This page should be static
+    </div>
+  )
+}
diff --git a/test/e2e/app-dir/ppr-full/ppr-full.test.ts b/test/e2e/app-dir/ppr-full/ppr-full.test.ts
index 0a070ba5f6b45..b95730f39e8ae 100644
--- a/test/e2e/app-dir/ppr-full/ppr-full.test.ts
+++ b/test/e2e/app-dir/ppr-full/ppr-full.test.ts
@@ -476,6 +476,18 @@ describe('ppr-full', () => {
             expect(revalidatedDynamicID).not.toBe(fallbackID)
           })
         })
+
+        /**
+         * This test is really here to just to force the the suite to have the expected route
+         * as part of the build. If this failed we'd get a build error and all the tests would fail
+         */
+        it('will allow dynamic fallback shells even when static is enforced', async () => {
+          const random = Math.random().toString(16).slice(2)
+          const pathname = `/fallback/dynamic/params/revalidate-${random}`
+
+          let $ = await next.render$(pathname)
+          expect($('[data-slug]').text()).toBe(`revalidate-${random}`)
+        })
       })
 
       it('should allow client layouts without postponing fallback if params are not accessed', async () => {