Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resets to initial state when there is Link with prefetch #388

Closed
Arctomachine opened this issue Nov 3, 2023 · 11 comments
Closed

Resets to initial state when there is Link with prefetch #388

Arctomachine opened this issue Nov 3, 2023 · 11 comments
Labels
bug Something isn't working next.js Issue is related to Next.js internals rather than this library

Comments

@Arctomachine
Copy link

Reproduction code: https://github.com/Arctomachine/usequerystate-bug

Only works in production (start) mode, not in dev

  1. pnpm install
  2. pnpm run build
  3. pnpm run start
  4. Got to index page /
  5. Click checkbox - page should flicker and reset to default value
  6. Click it again - should work as expected
  7. Move cursor over any link in grid - it should reset again

To "fix" it go to ./app/Wrapper.tsx, at line 33 change prefetch={true} to prefetch={false}

@franky47 franky47 added the bug Something isn't working label Nov 3, 2023
@franky47
Copy link
Member

franky47 commented Nov 4, 2023

Thanks for the reproduction. It looks like prefetch calls history.replaceState, which throws things off track, thinking it's an actual navigation. And since prefetch is a production-only feature, it explains why it works fine in development.

I'll see if that's a recent behaviour in the newest canaries, otherwise we might have to use a different strategy to differentiate external navigation from prefetches.

@franky47
Copy link
Member

franky47 commented Nov 4, 2023

I have yet to trace the full path taken from dispatching the ACTION_PREFETCH action in Next.js to the call to the history API that causes the problem, but it looks like Next.js is keeping an internal record of the URL it's "supposed" to be at, rather than using the actual location object.

A proof of this is using { shallow: false } for the query filter, which uses the Next.js router rather than a call to the history API. This solves both the checkbox blinking and the prefetch resets, because the router was told about the intent to have a querystring in the URL. Obviously, this is not a satisfying solution to our problem.

I'm not sure if the Next.js team would consider that a bug, but there may be other libraries interfacing with the history API that have a similar issue (eg: alternative routers).

@franky47
Copy link
Member

franky47 commented Nov 4, 2023

I can confirm this behaviour started to appear in 14.0.2-canary.7, specifically due to this PR: vercel/next.js#56497.

@franky47 franky47 added the next.js Issue is related to Next.js internals rather than this library label Nov 4, 2023
franky47 added a commit that referenced this issue Nov 7, 2023
This will pass up to 14.0.2-canary.7, which introduced a bug
with prefetch resetting shallow query updates.
@franky47
Copy link
Member

franky47 commented Nov 9, 2023

Note: 14.0.2 has been released with the PR that broke shallow URL updates when combined with prefetch links. next-usequerystate is unfortunately not compatible with this release (and any subsequent until I have identified the root cause and they patched it on their end, PR coming).

@damian-balas
Copy link

@franky47 Could you tell me if you plan to maintain this repo for a long period of time? I plan to use it, but I don't want libraries that will "die" in a few months. Would really appreciate if you could give me some feedback about your plans with this repo :)

Thanks! :)

@franky47
Copy link
Member

franky47 commented Nov 9, 2023

I'm actively using it across many production projects, it's not going anywhere. Ideally, I'd like to get some of the complexity pruned out by merging it into Next.js core (shallow routing in app router).

That being said, it's also dependent on external factors, like a stable revenue stream, which is not the case at the moment. Sponsors are welcome to help with maintenance.

@franky47
Copy link
Member

franky47 commented Nov 10, 2023

Found the root cause of the issue. Let's follow the N whys:

The main effect is, as described by @Arctomachine: Query updates get reset when hovering (or mounting) a prefetch Link.

  1. Why: on prefetch, history.replaceState is called with the internal canonicalUrl in the app router's HistoryUpdater component, which is not aware of the shallow URL update. (source)
  2. Why: the HistoryUpdater re-ran its useInsertionEffect.
  3. Why: the sync parameter function changed
  4. Why: it was recreated because the reducer state had changed (source)

The impacting PR did not change how the prefetch reducer behaves. It did not change the app router state when running in 14.0.2-canary.6 and below, and actually still doesn't change the internal state in subsequent releases.

What changed however, is that the reducer state is now based on Promises. For each action, a new Promise is created. Those are referencially different, even if the state they resolve to is the same. The sync function is being rebuilt at every action dispatched, because it's essentially a different reference.

Solution: drop this sync function entirely, as its purpose was to synchronize the Redux devtools store somehow, but I doubt it still serves its purpose with Promises.

Going to run this by the Next.js team, I'll update this comment with the PR number when it's out. Edit: vercel/next.js#58297

Notes: the sync (along with the whole Redux devtools code) was introduced in vercel/next.js#39866

Note: also keep an eye on these:

@franky47
Copy link
Member

@Arctomachine Could you please try the following combination, and let me know if you are still experiencing this issue?

  • next@14.0.3-canary.6
  • next-usequerystate@1.12.0-beta.1
  • Setting the experimental flag windowHistorySupport to true

@Arctomachine
Copy link
Author

It works now, tested on project where it first appeared - no problems so far.

@franky47
Copy link
Member

Thanks a lot for your feedback, I have released 1.12.0. Fingers crossed that PR on the Next.js repo lands on 12.0.3 🤞

kodiakhq bot pushed a commit to vercel/next.js that referenced this issue Nov 14, 2023
## 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`](https://github.com/47ng/next-usequerystate), which is used in the Vercel dashboard, and which is impacted by this change (see 
[#388](47ng/nuqs#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

- [x] 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.


Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>
@franky47
Copy link
Member

franky47 commented Nov 15, 2023

next@14.0.3-canary.8 has been released with the prefetch fix. The experimental flag is not needed anymore from there on (although its use is supported).

I'll publish next-usequerystate@1.12.1 with the updated semver range when next@12.0.3 lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working next.js Issue is related to Next.js internals rather than this library
Projects
None yet
Development

No branches or pull requests

3 participants