fix: Don't reset shallow URL updates on prefetch #58297

franky47 · 2023-11-10T13:08:34Z

Description

Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced in #56497 that turned the Redux store state into a Promise, rather than a synchronous state update.

This caused the sync function -- used to send state updates to the Redux Devtools -- to be recreated on every dispatch, which in turn, by referential instability, caused the HistoryUpdater component to re-render and trigger a history.replaceState with no particular change, but with the internal canonicalUrl.

When an app does a soft/shallow navigation by calling history methods directly (currently the only way to do shallow search params updates in the app router), these changes would have been overwritten by any prefetch (eg: hovering or mounting a Link), which is usually a no-op for the navigation state.

This PR changes the sync function to take the state as an argument rather than as a closure. The whole app router state is also unwrapped only once, and fed to the HistoryUpdater. Changes to its contents made by reducers will cause the HistoryUpdater effect to re-run, triggering history updates and a call to the sync function.

Context

I maintain next-usequerystate, which is used in the Vercel dashboard, and which is impacted by this change (see
#388).

History

@timneutkens introduced the sync function and the whole Redux devtools reducer in #39866, with the note:

a new hook useReducerWithReduxDevtools has been added, we'll probably want to put this behind a compile-time flag when the new router is marked stable but until then it's useful to have it enabled by default (only
when you have Redux Devtools installed ofcourse).

If a different direction is needed to keep sending RENDER_SYNC actions to Redux devtools, I'll be happy to rework this PR to move the sync function into the action queue.

Changes

Added e2e test. Requires a start mode as prefetch links are disabled in development. Test was verified to fail from next@>=12.0.2-canary.7 without the fix.

ztanner · 2023-11-10T17:23:03Z

Hey @franky47 -- thanks for working on this. We'd like to still keep the existing devtools sync behavior as we rely on this for debugging.

Let me know if you'd like to take a stab at that otherwise I can take a look next week!

franky47 · 2023-11-10T23:21:14Z

@ztanner thanks for the quick feedback, I'll have a look throughout the weekend and I'll let you know.

Ultimately, this is only a quick fix, a better fix for this behaviour would be to have access to shallow routing in the app router, to mirror the pages router API. It would allow keeping the internal canonicalUrl in sync with the actual window.location and re-renders of the HistoryUpdater wouldn't be a big deal (aside from whole page re-rendering perf issues).

I don't know if that's a thing you guys at Vercel have in the pipeline, but I'd love to take a closer look at how to do this (it would shave a good chunk of hacks off my library).

franky47 · 2023-11-13T12:54:15Z

@ztanner I have restored the Redux devtools sync function, with the caveat that it now runs when a state update resolves to a new reducer state, rather than when an action is dispatched. It seemed more inline with the previous synchronous behaviour.

There were a few places in the app router render body where a Promise reducer state would be unwrapped to parts of the app router state. I'm kind of new to the use of React.use(), does it make a difference vs unwrapping once and accessing parts where needed?

franky47 · 2023-11-13T14:45:21Z

Note: it's currently conflicting with #58335, which may make this PR irrelevant, will investigate further on the impacts on next-usequerystate.

ijjk · 2023-11-13T15:50:30Z

Stats from current PR

Default Build

General

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
buildDuration	10.8s	10.7s	N/A
buildDurationCached	6.2s	6.1s	N/A
nodeModulesSize	199 MB	199 MB	N/A
nextStartRea..uration (ms)	402ms	400ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
199-HASH.js gzip	29.2 kB	29.1 kB	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	N/A
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	243 B	239 B	N/A
main-HASH.js gzip	31.7 kB	31.8 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	✓
Overall change	46.9 kB	46.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	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	504 B	506 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.3 kB	4.3 kB	N/A
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.65 kB	2.65 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	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
_buildManifest.js gzip	486 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
index.html gzip	528 B	526 B	N/A
link.html gzip	540 B	541 B	N/A
withRouter.html gzip	524 B	521 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
edge-ssr.js gzip	92.5 kB	92.5 kB	N/A
page.js gzip	145 kB	145 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	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	24.8 kB	24.8 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	franky47/next.js fix/dont-reset-shallow-url-updates-on-prefetch	Change
app-page-exp...dev.js gzip	167 kB	167 kB	✓
app-page-exp..prod.js gzip	93.2 kB	93.2 kB	✓
app-page-tur..prod.js gzip	93.9 kB	93.9 kB	✓
app-page-tur..prod.js gzip	88.5 kB	88.5 kB	✓
app-page.run...dev.js gzip	137 kB	137 kB	✓
app-page.run..prod.js gzip	87.9 kB	87.9 kB	✓
app-route-ex...dev.js gzip	23.8 kB	23.8 kB	✓
app-route-ex..prod.js gzip	16.4 kB	16.4 kB	✓
app-route-tu..prod.js gzip	16.4 kB	16.4 kB	✓
app-route-tu..prod.js gzip	16 kB	16 kB	✓
app-route.ru...dev.js gzip	23.2 kB	23.2 kB	✓
app-route.ru..prod.js gzip	16 kB	16 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.8 kB	21.8 kB	✓
pages.runtim...dev.js gzip	22.5 kB	22.5 kB	✓
pages.runtim..prod.js gzip	21.8 kB	21.8 kB	✓
server.runti..prod.js gzip	48.8 kB	48.8 kB	✓
Overall change	922 kB	922 kB	✓

Diff details

Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for 199-HASH.js

Diff too large to display

Commit: 1b6d315

ijjk · 2023-11-13T15:50:52Z

Failing test suites

Commit: 1b6d315

pnpm test-dev test/e2e/app-dir/app-compilation/index.test.ts

app dir > HMR > should not cause error when removing loading.js

Expand output

● app dir › HMR › should not cause error when removing loading.js

TIMED OUT: hello from new page

Server Error

undefined

  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> (e2e/app-dir/app-compilation/index.test.ts:44:11)

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

anonkey · 2023-11-14T05:48:41Z

@franky47 I made the conflict resolution for testing knowing if using your library with this fix was okay on my project and opened a PR because some tests was failing on my local, feel free to cherry-pick and take commit ownership as it's your code.
#58417

franky47 · 2023-11-14T06:23:32Z

@anonkey there's actually a lot that changed behaviourally in 12.0.3-canary.6, I'm still looking into how to properly integrate it into nuqs. See 47ng/nuqs#394

Thanks for the conflict res, I'll take a look at your changes.

franky47 · 2023-11-14T09:59:09Z

The WHS flag does fix the issue described here, but it's still present when the flag isn't used, so this PR still makes sense for baseline behaviour.

@anonkey next-usequerystate@1.12.0-beta.1 now works with next@14.0.3-canary.6 with the windowHistorySupport flag turned on, could you give it a try please?

franky47 · 2023-11-14T13:25:19Z

Conflicts resolved. @ztanner could you take a look please? 12.0.3-canary.6 brought a solution for shallow routing, but without the experimental windowHistorySupport flag, the prefetch bug is still blocking further next-usequerystate updates against next@latest.

Between 14.0.2-canary.6 and 14.0.2-canary.7, a change was introduced in #56497 that turned the Redux store state into Promises, rather than a synchronous state update. This caused the `sync` function used to send state update to the Redux Devtools to be recreated on every dispatch, which in turn, by referential instability, caused the `HistoryUpdater` component to re-render and trigger a `history.replaceState` with no particular change, but with the internal `canonicalUrl`. When an app does a soft/shallow navigation by calling history methods directly (currently the only way to do shallow search params updates in the app router), these changes would have been overwritten by any prefetch (eg: hovering or mounting a Link), which is usually a no-op for the navigation state. This commit removes the `sync` function for the Redux devtools, as it cannot function as-is: actual state updates need to be awaited and forwarded somewhere else in the router if this behaviour is to be maintained, and should be decoupled from history calls to prevent side-effects.

- Add missing types for sync function & HistoryUpdater props - Make the `sync` function referentially stable - Unwrap reducer state only once in app router

…renced

ztanner

The changes looks good, thanks for working on this!

…etch

anonkey · 2023-11-20T14:34:53Z

The WHS flag does fix the issue described here, but it's still present when the flag isn't used, so this PR still makes sense for baseline behaviour.

@anonkey next-usequerystate@1.12.0-beta.1 now works with next@14.0.3-canary.6 with the windowHistorySupport flag turned on, could you give it a try please?

Nice thank you

franky47 requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners November 10, 2023 13:08

ijjk added area: tests type: next labels Nov 10, 2023

franky47 mentioned this pull request Nov 10, 2023

Resets to initial state when there is Link with prefetch 47ng/nuqs#388

Closed

franky47 mentioned this pull request Nov 10, 2023

wrong url after refresh page 47ng/nuqs#392

Closed

franky47 mentioned this pull request Nov 11, 2023

Roadmap / Changelog 47ng/nuqs#375

Open

ztanner added the CI approved Approve running CI for fork label Nov 13, 2023

anonkey mentioned this pull request Nov 14, 2023

fix: Don't reset shallow URL updates on prefetch #58417

Closed

franky47 mentioned this pull request Nov 14, 2023

feat: Add support for experimental WHS 47ng/nuqs#394

Merged

franky47 and others added 5 commits November 14, 2023 15:49

fix: Restore Redux devtools sync

79e20cd

- Add missing types for sync function & HistoryUpdater props - Make the `sync` function referentially stable - Unwrap reducer state only once in app router

chore: Fix linter (unused type)

7bbd777

remove top-level use in favor of calling it where it needs to be refe…

1ef63bd

…renced

relocate test

9c88d94

undo import diffs

3c5a3b7

ztanner approved these changes Nov 14, 2023

View reviewed changes

Merge branch 'canary' into fix/dont-reset-shallow-url-updates-on-pref…

16b7dfc

…etch

kodiakhq bot merged commit e4158f6 into vercel:canary Nov 14, 2023
59 checks passed

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Don't reset shallow URL updates on prefetch #58297

fix: Don't reset shallow URL updates on prefetch #58297

franky47 commented Nov 10, 2023 •

edited

Loading

ztanner commented Nov 10, 2023

franky47 commented Nov 10, 2023 •

edited

Loading

franky47 commented Nov 13, 2023 •

edited

Loading

franky47 commented Nov 13, 2023

ijjk commented Nov 13, 2023

ijjk commented Nov 13, 2023

anonkey commented Nov 14, 2023

franky47 commented Nov 14, 2023

franky47 commented Nov 14, 2023

franky47 commented Nov 14, 2023

ztanner left a comment

anonkey commented Nov 20, 2023 •

edited

Loading

fix: Don't reset shallow URL updates on prefetch #58297

fix: Don't reset shallow URL updates on prefetch #58297

Conversation

franky47 commented Nov 10, 2023 • edited Loading

Description

Context

History

Changes

ztanner commented Nov 10, 2023

franky47 commented Nov 10, 2023 • edited Loading

franky47 commented Nov 13, 2023 • edited Loading

franky47 commented Nov 13, 2023

ijjk commented Nov 13, 2023

Stats from current PR

ijjk commented Nov 13, 2023

Failing test suites

anonkey commented Nov 14, 2023

franky47 commented Nov 14, 2023

franky47 commented Nov 14, 2023

franky47 commented Nov 14, 2023

ztanner left a comment

Choose a reason for hiding this comment

anonkey commented Nov 20, 2023 • edited Loading

franky47 commented Nov 10, 2023 •

edited

Loading

franky47 commented Nov 10, 2023 •

edited

Loading

franky47 commented Nov 13, 2023 •

edited

Loading

anonkey commented Nov 20, 2023 •

edited

Loading