Ensure original history is read in effect #58861

timneutkens · 2023-11-24T13:19:35Z

What?

Fixes #58335 (comment). Waiting on a reply for the exact case but my assumption is that the history is overwritten in the user module instead of before the other JS loads.

timneutkens · 2023-11-24T13:19:46Z

Current dependencies on/for this PR:

canary
- PR Ensure original history is read in effect #58861 👈

This stack of pull requests is managed by Graphite.

ijjk · 2023-11-24T13:25:09Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	Change
buildDuration	10.6s	10.4s	N/A
buildDurationCached	6s	6.6s	⚠️ +586ms
nodeModulesSize	199 MB	199 MB	⚠️ +1.59 kB
nextStartRea..uration (ms)	422ms	423ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	Change
199-HASH.js gzip	30.8 kB	30.7 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	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	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	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	Change
_buildManifest.js gzip	484 B	483 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	Change
index.html gzip	529 B	527 B	N/A
link.html gzip	539 B	542 B	N/A
withRouter.html gzip	525 B	522 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	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	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	Change
middleware-b..fest.js gzip	625 B	626 B	N/A
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	1.92 kB	1.92 kB	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js 11-24-Ensure_original_history_is_read_in_effect	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	✓

Diff details

Diff for page.js

Diff too large to display

Diff for 199-HASH.js

Diff too large to display

Commit: 55dac4a

franky47 · 2023-12-01T05:34:47Z

Confirmed in the case of next-usequerystate: patching there is done at the top level of the module, so the sequence of events before this PR (14.0.3 and canaries above) is:

App router top level evaluates, copying history.replaceState into originalReplaceState
next-usequerystate module loads and top level evaluates, patching history.replaceState with its own pass-through
App router useEffect runs and assigns history.replaceState to originalReplaceState, effectively erasing the previous patch.

I patched my local copy of Next.js 14.0.3 with this PR and it does solve the issue.

Edit: when the windowHistorySupport flag is set to true, this PR causes an infinite loop of shallow state updates on navigation (nothing related to my lib, it happens between two stock pages with a <Link>). The first log line is hit 200 times before the history API rate limit is hit and throws an error:

07:11:46.376 Warning: useInsertionEffect must not schedule updates.
  HistoryUpdater@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/components/app-router.js:64:36
  Router@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/components/app-router.js:170:100
  ErrorBoundaryHandler@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/components/error-boundary.js:100:9
  ErrorBoundary@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/components/error-boundary.js:130:67
  AppRouter@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/components/app-router.js:491:47
  ServerRoot@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/app-index.js:128:24
  RSCComponent
  Root@webpack-internal:///(app-pages-browser)/../../node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/app-index.js:144:24 199 [app-index.js:32:21](webpack://_N_E/node_modules/.pnpm/next@14.0.4-canary.32_react-dom@18.2.0_react@18.2.0/node_modules/next/dist/client/app-index.js?7251)
07:11:47.386 Too many calls to Location or History APIs within a short timeframe.

…ffect

timneutkens · 2023-12-01T08:45:47Z

@franky47 good catch, now I remember why I did the original implementation in the first place, Next.js internal push/replace should not trigger the patched behavior. Will add a marker for it.

ijjk · 2023-12-01T08:50:32Z

Failing test suites

Commit: 55dac4a

pnpm test test/integration/clean-distdir/test/index.test.js

Cleaning distDir > disabled write > should not clean up .next before build start

Expand output

● Cleaning distDir › disabled write › should not clean up .next before build start

thrown: "Exceeded timeout of 60000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  50 |     })
  51 |
> 52 |     it('should not clean up .next before build start', async () => {
     |     ^
  53 |       await checkFileWrite(true)
  54 |     })
  55 |   })

  at it (integration/clean-distdir/test/index.test.js:52:5)
  at describe (integration/clean-distdir/test/index.test.js:36:3)
  at Object.describe (integration/clean-distdir/test/index.test.js:31:1)

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

…ffect

packages/next/src/client/components/app-router.tsx

franky47 · 2023-12-02T13:54:53Z

That last commit solved all known issues in next-usequerystate, both with and without the windowHistorySupport flag, thanks @timneutkens !

If that PR could make it to 14.0.4, that'd be awesome. 🙏

timneutkens · 2023-12-02T19:03:35Z

@franky47 thanks for checking! It'll be in the next stable release yeah 👍

Ensure original history is read in effect

941b1b1

timneutkens requested review from ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners November 24, 2023 13:19

ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Nov 24, 2023

timneutkens mentioned this pull request Nov 24, 2023

Add experimental support for history.pushState and history.replaceState #58335

Merged

shuding approved these changes Nov 24, 2023

View reviewed changes

ztanner approved these changes Nov 25, 2023

View reviewed changes

Merge branch 'canary' into 11-24-Ensure_original_history_is_read_in_e…

0d948ec

…ffect

timneutkens marked this pull request as draft December 1, 2023 08:45

timneutkens marked this pull request as ready for review December 1, 2023 08:51

Avoid loop when triggering push/replace internally

95353f5

andrewmumblebee mentioned this pull request Dec 1, 2023

NextTopLoader stopped working on Nextjs 14.0.3 TheSGJ/nextjs-toploader#56

Open

timneutkens added 2 commits December 1, 2023 15:18

Merge branch 'canary' into 11-24-Ensure_original_history_is_read_in_e…

6363e27

…ffect

Merge branch 'canary' into 11-24-Ensure_original_history_is_read_in_e…

34a0bfa

…ffect

franky47 suggested changes Dec 2, 2023

View reviewed changes

packages/next/src/client/components/app-router.tsx Outdated Show resolved Hide resolved

Call original

ffb01cb

Fix checks

55dac4a

ztanner approved these changes Dec 2, 2023

View reviewed changes

This was referenced Dec 2, 2023

fix: Routing misses searchParams on mount 47ng/nuqs#417

Merged

fix: Push 47ng/nuqs#419

Merged

URL query params are not detected when navigating with the Next Link component 47ng/nuqs#416

Closed

timneutkens merged commit 8ff1368 into canary Dec 3, 2023
66 of 68 checks passed

timneutkens deleted the 11-24-Ensure_original_history_is_read_in_effect branch December 3, 2023 08:18

polynook mentioned this pull request Dec 3, 2023

POST request is not fired when clicking on links 4lejandrito/next-plausible#114

Closed

This was referenced Dec 3, 2023

issue nextjs version 14.0.3 not finish loader TheSGJ/nextjs-toploader#58

Closed

So how is this supposed to work? TheSGJ/nextjs-toploader#57

Closed

Toploader gets stuck when clicked on external links TheSGJ/nextjs-toploader#59

Closed

This was referenced Dec 7, 2023

Known issues & solutions / Version compatibility table 47ng/nuqs#423

Open

Roadmap / Changelog 47ng/nuqs#375

Open

AkifumiSato mentioned this pull request Dec 14, 2023

update example's next.js@v14.0.4 recruit-tech/location-state#167

Merged

github-actions bot added the locked label Dec 17, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure original history is read in effect #58861

Ensure original history is read in effect #58861

timneutkens commented Nov 24, 2023

timneutkens commented Nov 24, 2023

ijjk commented Nov 24, 2023 •

edited

franky47 commented Dec 1, 2023 •

edited

timneutkens commented Dec 1, 2023

ijjk commented Dec 1, 2023 •

edited

franky47 commented Dec 2, 2023

timneutkens commented Dec 2, 2023

Ensure original history is read in effect #58861

Ensure original history is read in effect #58861

Conversation

timneutkens commented Nov 24, 2023

What?

timneutkens commented Nov 24, 2023

ijjk commented Nov 24, 2023 • edited

Stats from current PR

franky47 commented Dec 1, 2023 • edited

timneutkens commented Dec 1, 2023

ijjk commented Dec 1, 2023 • edited

Failing test suites

franky47 commented Dec 2, 2023

timneutkens commented Dec 2, 2023

ijjk commented Nov 24, 2023 •

edited

franky47 commented Dec 1, 2023 •

edited

ijjk commented Dec 1, 2023 •

edited