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

[BUG] [v7] Location update is out of sync with the rendering cycle #12552

Closed
KurtGokhan opened this issue Dec 14, 2024 · 6 comments
Closed

[BUG] [v7] Location update is out of sync with the rendering cycle #12552

KurtGokhan opened this issue Dec 14, 2024 · 6 comments
Labels

Comments

@KurtGokhan
Copy link

KurtGokhan commented Dec 14, 2024

I'm using React Router as a...

library

Reproduction

Visit https://stackblitz.com/edit/vitejs-vite-fxh9ikze?file=src%2FApp.tsx

Click Increment button and observe the console logs. The code detects if there is a state discrepancy, and logs to the console.

System Info

It is happening in v7 regardless of the OS or browser. Here is the envinfo for the Stackblitz example.

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    react-router: ^7.0.2 => 7.0.2 
    vite: ^6.0.3 => 6.0.3

Used Package Manager

npm

Expected Behavior

To be able to properly use the router as a state manager, the state must be in sync with the React rendering process. So when I call the setter of useSearchParams, it should update the search params in the next render cycle, similar to how useState works.

As a long time user of v5 and v6, I think this was working in all versions before v7. (Test it here)

Actual Behavior

It seems like LocationContext is updating one frame late. It may not be an issue specific to useSearchParams but maybe useLocation in general. I have tried flushSync and even awaiting the promise returned by the navigation callback, but the issue still happens.

@KurtGokhan KurtGokhan added the bug label Dec 14, 2024
@joshkel
Copy link

joshkel commented Dec 14, 2024

This may be related to v7's use of React transitions for LocationContext. If I take your v6 demo and change it to use <RouterProvider router={router} future={{v7_startTransition: true}} />, I get the same "States are not in sync" error.

@timdorr
Copy link
Member

timdorr commented Dec 15, 2024

Yes, that is correct. The location is updated within a transition to support Suspense and other async React features. If you add a else console.log('States are in sync.', { countState, countSearch }); to your code, you'll notice it renders a second time after the states are in sync.

If you wrap your own setState calls in a transition, it will only render once (because React will batch the renders automatically) and will always be in sync: startTransition(() => setCountState((c) => c + 1))

@timdorr timdorr closed this as completed Dec 15, 2024
@KurtGokhan
Copy link
Author

KurtGokhan commented Dec 15, 2024

The new behavior has a lot of race condition potential. Is there a way to opt-out of this, instead of just accepting "that is how it works now"? If disabling it isn't an option, is there a way to get a callback or Promise when the transition is committed? What about the promise returned by the navigate function? Also I was under the impression flushSync would cause transition to commit immediately.

I don't want to sound anti-progressive, but with the new features like transitions, it seems like the internals of React are leaking more than ever. Now every code I write needs to be wary of transitions because the library I am using may or may not be using transitions, even if I don't need them. I could use the browser history as a reliable state manager before, but now I cannot.

Couldn't there be an option to not use transitions if I am not using loaders and other async stuff with the router?

Edit:

If you wrap your own setState calls in a transition, it will only render once (because React will batch the renders automatically) and will always be in sync: startTransition(() => setCountState((c) => c + 1))

This may not work in the future according to the docs:

If there are multiple ongoing Transitions, React currently batches them together. This is a limitation that may be removed in a future release.

@timdorr
Copy link
Member

timdorr commented Dec 15, 2024

Our hooks now return Promises, so yes, you'll be able to use the use hook and new React 19 APIs to better handle these things in a more specific manner.

@KurtGokhan
Copy link
Author

@timdorr when you say "you'll be able" do you mean it will come in a future version, or should it already work? Are we talking about the same issue here? I am talking about a fundamental issue and your short explanation doesn't give me confidence. It is outright impossible to use the query params as a proper state manager now.

Awaiting the promise returned from navigate doesn't ensure the transition is finished. It doesn't even work when I pass flushSync: true. And flushSync should eliminate any non-sync behavior in my opinion. It seems to me like there should be a way to either set or get the url state synchronously. Any workaround will not be reliable.

Also there is this line in startTransition docs:

If there are multiple ongoing Transitions, React currently batches them together. This is a limitation that may be removed in a future release.

So even the workaround you suggested isn't a future-proof way to do it.

@KurtGokhan
Copy link
Author

KurtGokhan commented Dec 24, 2024

I brought myself up to date with the concurrency in React. I already had some information about them but I wasn't aware of the whole implications until I used them in a real application.

startTransition is not a silver bullet and it shouldn't be used blindly. There are cases where it is perfectly valid to disable it. Things that happened instantly before don't happen instantly anymore. It is a huge tradeoff in terms of complexity when the old way used to work perfectly for some use cases (most, considering we didn't have this feature before).

It is possible to make your entire app transition-aware. But this is too impractical, and often unnecessary. From my experience, transition-aware hooks/components are not easily composable. Also there are some bugs and unexpected behaviors that make it harder (examples 1, 2, and all caveats here).

Transitions are a feature of React, but so is useDeferredValue and useSyncExternalStore. I think it's not good to fixate on one solution and ignore others. You don't have to sacrifice old features to add new features.

I could keep using v6. Or I could stop overthinking and ignore sub-second state tearing issues and tiny lags like most of the slop out there. But I am asking you to reconsider, for the greater good. I am sure more people will (or worse, won't) notice these issues as they keep migrating to v7.

I am aware I made too much noise, but TLDR:

There should be a way to opt-out of startTransition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants