Fix top down `generateStaticParams` in `page` component #55912

viktorronnback · 2023-09-24T18:18:31Z

What?

Why?

Exporting generateStaticParams from nested dynamic routes only works if "parent" routes exports from the layout component.

How?

Added page component of "parent" routes (only dynamic routes) to tree code in loader.

Add test for export from parent page
Add upper level pages to tree code

orca-security-us

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Secrets	0 0 0 0	View in Orca

viktorronnback · 2023-09-24T18:23:30Z

packages/next/src/build/webpack/loaders/next-app-loader.ts

+      const fileTypesToResolve: (FileTypeValues | 'page')[] =
+        Object.values(FILE_TYPES)
+
+      // Special case for exports in parent pages


This just fixes a special case (only for dynamic routes). Might be better to do a more extensive code rewrite on how pages in parent routes are added to the tree code for other exports in parent routes to work (if there are any such cases).

…page exist on parent route

viktorronnback · 2023-09-25T13:42:24Z

packages/next/src/build/utils.ts

+  let { generateStaticParams } = mod || {}
+  const { getStaticPaths } = mod || {}
+
+  // Look for generateStaticParams in template or page if not already found


Currently it prioritizes exports in layout, then template and lastly page. Except for if the route does not contain a layout, then it prioritizes page. Once it has found generateStaticParams on a certain route level, it ignores all other exports for that level.

A better solution would be to check if generateStaticParams is exported from multiple files in the same route, and then throw an error. However this requires all these files in the route to be imported (layout, template, page) making for slower build times. Is it worth the trade-off to get good error messages?

viktorronnback · 2023-10-09T09:16:05Z

@ijjk @leerob Is anyone taking a look at this? Please comment on how I should change the PR for it to be accepted, or if you are working on some alternative fix!

ijjk · 2023-11-08T21:59:14Z

Allow CI Workflow Run

approve CI run for commit: 6a93666

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

ijjk · 2023-11-08T21:59:15Z

Allow CI Workflow Run

approve CI run for commit: dba94be

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

viktorronnback · 2023-11-29T20:35:45Z

@timneutkens @ijjk @shuding @huozhi @ztanner @feedthejim @wyattjoh It would be great to get some feedback on this PR, we are a few that reacted on the original issue (#53717) by now. Even though there is a pretty easy workaround, I think it would be great to get this issue fixed in some way to avoid confusion by users. Of course don't hesitate to tell me if you want this fix implemented in some other way.

ztanner

Thanks for working on this fix! I've left some initial comments

ztanner · 2023-12-04T18:03:28Z

packages/next/src/build/utils.ts

+  if (isDynamicSegment && !generateStaticParams) {
+    ;({ generateStaticParams } = (await segment[2]?.template?.[0]?.()) || {})
+
+    if (!generateStaticParams && isLayout)
+      ({ generateStaticParams } = (await segment[2]?.page?.[0]?.()) || {})
+  }


Since we're already checking for generateStaticParams in the lines above I think we could probably simplify this to something like:

Suggested change

if (isDynamicSegment && !generateStaticParams) {

;({ generateStaticParams } = (await segment[2]?.template?.[0]?.()) || {})

if (!generateStaticParams && isLayout)

({ generateStaticParams } = (await segment[2]?.page?.[0]?.()) || {})

}

if (isDynamicSegment && !generateStaticParams) {

if (isLayout) {

;({ generateStaticParams } = (await segment[2]?.page?.[0]?.()) || {})

} else {

;({ generateStaticParams } = (await segment[2]?.template?.[0]?.()) || {})

}

}

ztanner · 2023-12-04T18:34:21Z

packages/next/src/build/webpack/loaders/next-app-loader.ts

+        Object.values(FILE_TYPES)
+
+      // Special case for exports in parent pages
+      const isSegmentDynamic = /^\[.*\]$/.test(normalizedParallelSegments[0])


Might be best to use our isDynamicRoute util for this check

ztanner · 2023-12-04T18:36:47Z

packages/next/src/build/utils.ts

+    ;({ generateStaticParams } = (await segment[2]?.template?.[0]?.()) || {})
+
+    if (!generateStaticParams && isLayout)
+      ({ generateStaticParams } = (await segment[2]?.page?.[0]?.()) || {})


unrelated to your changes, but can we use the LoaderTree type for the segment argument in this function? That way we can get some typesafety here

ztanner · 2023-12-04T19:19:04Z

packages/next/src/build/webpack/loaders/next-app-loader.ts

+      // Special case for exports in parent pages
+      const isSegmentDynamic = /^\[.*\]$/.test(normalizedParallelSegments[0])
+      const isFinalSegment =
+        normalizedParallelSegments[0] === page.split('/').at(-2)


Had to remind myself why this would be -2 which was a bit confusing. Perhaps we can first run page through normalizeAppPath -- that'll give us something like /gen-params-top-down-page/[lang]/[slug]. Then we can simplify this check to be normalizedParallelSegments[0] === normalizedAppPath.at(-1)

ztanner · 2023-12-04T19:27:00Z

packages/next/src/build/webpack/loaders/next-app-loader.ts

@@ -302,9 +304,18 @@ async function createTreeCodeFromPath(

      const parallelSegmentPath = subSegmentPath.join('/')

+      const fileTypesToResolve: (FileTypeValues | 'page')[] =


since we're creating a separate type for this, I think it'd make more sense to add the | 'page' to FileTypeValues rather than inlining it

ijjk · 2023-12-04T19:31:07Z

Failing test suites

Commit: a268c4e

pnpm test-start test/e2e/app-dir/hooks/hooks.test.ts

app dir - hooks > useSelectedLayoutSegment > should return null in pages
app dir - hooks > useSelectedLayoutSegments > should return an empty array in pages

Expand output

● app dir - hooks › useSelectedLayoutSegments › should return an empty array in pages

SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)

  151 |         )
  152 |
> 153 |         expect(JSON.parse($('#page-layout-segments').text())).toEqual([])
      |                     ^
  154 |       })
  155 |     })
  156 |

  at Object.parse (e2e/app-dir/hooks/hooks.test.ts:153:21)

● app dir - hooks › useSelectedLayoutSegment › should return null in pages

SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)

  180 |         )
  181 |
> 182 |         expect(JSON.parse($('#page-layout-segment').text())).toEqual(null)
      |                     ^
  183 |       })
  184 |     })
  185 |   }

  at Object.parse (e2e/app-dir/hooks/hooks.test.ts:182:21)

Read more about building and testing Next.js in contributing.md.

pnpm test-dev test/development/middleware-errors/index.test.ts

middleware - development errors > when there is a compilation error from boot > logs the error correctly

Expand output

● middleware - development errors › when there is a compilation error from boot › logs the error correctly

TIMED OUT: success

undefined

Error: expect(received).toEqual(expected) // deep equality

Expected: 2
Received: 3

  626 |
  627 |   if (hardError) {
> 628 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
      |           ^
  629 |   }
  630 |   return false
  631 | }

  at check (lib/next-test-utils.ts:628:11)
  at Object.<anonymous> (development/middleware-errors/index.test.ts:250:9)

Read more about building and testing Next.js in contributing.md.

pnpm test-start test/e2e/app-dir/use-params/use-params.test.ts

use-params > should work for nested dynamic params
use-params > should work for nested dynamic params client navigating

Expand output

● use-params › should work for nested dynamic params

expect(received).toBe(expected) // Object.is equality

Expected: "b"
Received: ""

  14 |       const $ = await next.render$('/a/b')
  15 |       expect($('#param-id').text()).toBe('a')
> 16 |       expect($('#param-id2').text()).toBe('b')
     |                                      ^
  17 |     })
  18 |
  19 |     it('should work for catch all params', async () => {

  at Object.toBe (e2e/app-dir/use-params/use-params.test.ts:16:38)

● use-params › should work for nested dynamic params client navigating

page.waitForSelector: Timeout 60000ms exceeded.
=========================== logs ===========================
waiting for locator('#param-id2')
============================================================

  362 |     return this.chain(() => {
  363 |       return page
> 364 |         .waitForSelector(selector, { timeout, state: 'attached' })
      |          ^
  365 |         .then(async (el) => {
  366 |           // it seems selenium waits longer and tests rely on this behavior
  367 |           // so we wait for the load event fire before returning

  at waitForSelector (lib/browsers/playwright.ts:364:10)

Read more about building and testing Next.js in contributing.md.

ijjk · 2023-12-04T19:34:02Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
buildDuration	10.5s	10.3s	N/A
buildDurationCached	5.8s	6.6s	⚠️ +814ms
nodeModulesSize	199 MB	199 MB	⚠️ +7.47 kB
nextStartRea..uration (ms)	420ms	424ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
199-HASH.js gzip	30.6 kB	30.6 kB	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	✓
494.HASH.js gzip	180 B	181 B	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	241 B	239 B	N/A
main-HASH.js gzip	31.7 kB	31.7 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	✓
Overall change	100 kB	100 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	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	501 B	503 B	N/A
css-HASH.js gzip	322 B	323 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	✓
edge-ssr-HASH.js gzip	253 B	255 B	N/A
head-HASH.js gzip	348 B	347 B	N/A
hooks-HASH.js gzip	369 B	368 B	N/A
image-HASH.js gzip	4.27 kB	4.27 kB	N/A
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.61 kB	2.6 kB	N/A
routerDirect..HASH.js gzip	311 B	311 B	✓
script-HASH.js gzip	384 B	383 B	N/A
withRouter-HASH.js gzip	307 B	308 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.17 kB	3.17 kB	✓

Client Build Manifests

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
_buildManifest.js gzip	484 B	483 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
index.html gzip	528 B	527 B	N/A
link.html gzip	538 B	540 B	N/A
withRouter.html gzip	524 B	522 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
edge-ssr.js gzip	92.6 kB	92.7 kB	N/A
page.js gzip	145 kB	145 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
middleware-b..fest.js gzip	626 B	626 B	✓
middleware-r..fest.js gzip	150 B	151 B	N/A
middleware.js gzip	35.7 kB	35.7 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.55 kB	2.55 kB	✓

Next Runtimes

	vercel/next.js canary	viktorronnback/next.js generate-static-params-page	Change
app-page-exp...dev.js gzip	168 kB	168 kB	✓
app-page-exp..prod.js gzip	93.8 kB	93.8 kB	✓
app-page-tur..prod.js gzip	94.5 kB	94.5 kB	✓
app-page-tur..prod.js gzip	89.1 kB	89.1 kB	✓
app-page.run...dev.js gzip	138 kB	138 kB	✓
app-page.run..prod.js gzip	88.4 kB	88.4 kB	✓
app-route-ex...dev.js gzip	24.2 kB	24.2 kB	✓
app-route-ex..prod.js gzip	16.8 kB	16.8 kB	✓
app-route-tu..prod.js gzip	16.9 kB	16.9 kB	✓
app-route-tu..prod.js gzip	16.4 kB	16.4 kB	✓
app-route.ru...dev.js gzip	23.6 kB	23.6 kB	✓
app-route.ru..prod.js gzip	16.4 kB	16.4 kB	✓
pages-api-tu..prod.js gzip	9.37 kB	9.37 kB	✓
pages-api.ru...dev.js gzip	9.64 kB	9.64 kB	✓
pages-api.ru..prod.js gzip	9.37 kB	9.37 kB	✓
pages-turbo...prod.js gzip	21.9 kB	21.9 kB	✓
pages.runtim...dev.js gzip	22.6 kB	22.6 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.3 kB	49.3 kB	✓
Overall change	930 kB	930 kB	✓

Commit: a268c4e

viktorronnback added 2 commits September 24, 2023 19:55

Add test for export from parent page

8eadffc

Add upper level pages to tree code

9cd8936

ijjk added area: tests type: next labels Sep 24, 2023

orca-security-us bot reviewed Sep 24, 2023

View reviewed changes

viktorronnback commented Sep 24, 2023

View reviewed changes

viktorronnback added 2 commits September 25, 2023 15:24

Add support for generateStaticParams in template and when layout and …

3c9846f

…page exist on parent route

Add test for template

5b3e8fd

viktorronnback commented Sep 25, 2023

View reviewed changes

viktorronnback marked this pull request as ready for review September 25, 2023 21:08

viktorronnback requested review from timneutkens, ijjk, shuding, huozhi, ztanner, a team, feedthejim and wyattjoh as code owners September 25, 2023 21:08

Merged canary and updated tests

6a93666

viktorronnback added 3 commits November 16, 2023 12:49

Merge branch 'canary' into generate-static-params-page

3d55a62

Merge branch 'canary' into generate-static-params-page

5a19d0f

Merge branch 'canary' into generate-static-params-page

dba94be

Merge branch 'canary' into generate-static-params-page

a268c4e

ztanner reviewed Dec 4, 2023

View reviewed changes

ztanner added the CI approved Approve running CI for fork label Dec 4, 2023

samcx removed the CI approved Approve running CI for fork label Mar 9, 2024

tylkofarci approved these changes Apr 19, 2024

View reviewed changes

balazsorban44 added tests and removed area: tests labels Apr 30, 2024

zto-sbenning mentioned this pull request Jul 20, 2024

Feature Request: Allowing "Generate params from the top down" for route with multiple dynamic segments svobik7/next-roots#242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix top down `generateStaticParams` in `page` component #55912

Fix top down `generateStaticParams` in `page` component #55912

viktorronnback commented Sep 24, 2023

orca-security-us bot left a comment •

edited

Loading

viktorronnback Sep 24, 2023 •

edited

Loading

viktorronnback Sep 25, 2023 •

edited

Loading

viktorronnback commented Oct 9, 2023

ijjk commented Nov 8, 2023

ijjk commented Nov 8, 2023 •

edited

Loading

viktorronnback commented Nov 29, 2023

ztanner left a comment

ztanner Dec 4, 2023

ztanner Dec 4, 2023

ztanner Dec 4, 2023

ztanner Dec 4, 2023

ztanner Dec 4, 2023

ijjk commented Dec 4, 2023 •

edited

Loading

ijjk commented Dec 4, 2023

		@@ -302,9 +304,18 @@ async function createTreeCodeFromPath(

		const parallelSegmentPath = subSegmentPath.join('/')

		const fileTypesToResolve: (FileTypeValues \| 'page')[] =

Fix top down generateStaticParams in page component #55912

Are you sure you want to change the base?

Fix top down generateStaticParams in page component #55912

Conversation

viktorronnback commented Sep 24, 2023

What?

Why?

How?

orca-security-us bot left a comment • edited Loading

Choose a reason for hiding this comment

Orca Security Scan Summary

viktorronnback Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

viktorronnback Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

viktorronnback commented Oct 9, 2023

ijjk commented Nov 8, 2023

ijjk commented Nov 8, 2023 • edited Loading

viktorronnback commented Nov 29, 2023

ztanner left a comment

Choose a reason for hiding this comment

ztanner Dec 4, 2023

Choose a reason for hiding this comment

ztanner Dec 4, 2023

Choose a reason for hiding this comment

ztanner Dec 4, 2023

Choose a reason for hiding this comment

ztanner Dec 4, 2023

Choose a reason for hiding this comment

ztanner Dec 4, 2023

Choose a reason for hiding this comment

ijjk commented Dec 4, 2023 • edited Loading

Failing test suites

ijjk commented Dec 4, 2023

Stats from current PR

Fix top down `generateStaticParams` in `page` component #55912

Fix top down `generateStaticParams` in `page` component #55912

orca-security-us bot left a comment •

edited

Loading

viktorronnback Sep 24, 2023 •

edited

Loading

viktorronnback Sep 25, 2023 •

edited

Loading

ijjk commented Nov 8, 2023 •

edited

Loading

ijjk commented Dec 4, 2023 •

edited

Loading