Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
franky47
added
the
feature/time-safety
commit: |
franky47
added
adapters/next/app
On shallow: true, this caused the optimistic state to be updated after the queue has reset, and caused the 1099 issue where the state would be temporarily reset to the previous value by the hook's useEffect sync system.
franky47
added
adapters/remix
| const [optimisticSearchParams, setOptimisticSearchParams] = | ||
| useOptimistic<URLSearchParams>(searchParams) | ||
| const updateUrl: UpdateUrlFunction = useCallback((search, options) => { | ||
| const queuedThrottledKeys = globalThrottleQueue.reset() |
There was a problem hiding this comment.
Removing this reset of the global throttle queue before the search params are updated is certainly a solution to the problem. However, as far as I understood the logic this was the only place in the App Router setup where the queue was reset once the params are flushed to the URL. This adapter provides autoResetQueueOnUpdate: false, so it will not reset the queue automatically. Without this reset here, will it still reset the queue in some other place I have missed?
There was a problem hiding this comment.
Yes, the mutex system counts how many calls of the history API have been made, and resets the queue once all of Next.js' internal calls have been made. We had those two resets in a race conditions before.
There was a problem hiding this comment.
You were right: the queue is indeed not reset 😓
I feel like this is the only way we can get both the 1099 and render-count tests to pass at the same time, reliably.
The queue is cleared on user navigation, so it doesn't grow infinitely and stays in sync with the URL, but it feels wrong to leave it pending after the URL has updated. The issue being that:
- If we clear it too early, we get the 1099 fail (the optimistic state flickers out when a high-priority state update happens in parallel)
- If we clear it too late, we get an extra render
There was a problem hiding this comment.
Something that I was wondering about is: why do we always use a transition to update the URL? I assume this is necessary to make useOptimistic work as intended in case the option shallow: false is set.
But do we actually have to wrap all the update logic in startTransition if we have shallow: true?
My assumption is that the deprioritized state update in the transition is causing the flicker when there are other non-transition state updates involved in the component. Maybe you could try using startTransition only if shallow: false is set? Hopefully, this causes useSearchParams to return the updated URL search params directly on next render.
There was a problem hiding this comment.
The issue was not much with the transition itself (we had that mechanism before the queue refactor in 2.5.0, for an issue that @aurorascharff found when pairing nuqs with startTransition & slow RSCs, see #718).
Since the refactor though, the optimistic state stored in uSES was either reset too early (causing the flicker) or too late (causing an extra re-render).
I found a better solution I think: reset the queue on the next push, just before we start loading it again. We now mark the queue for "ready to be reset" when resolving the returned Promise (URL has been updated, or at least requested to be). The next push occurs synchronously (same event loop tick) as the local, optimistic state update, and so the reset of the old pending query doesn't cause an extra render, and doesn't flicker as they both switch at the same time.
There may be cases where this falls apart, but as long as JavaScript is single-threaded, I think it might be the best solution until we refactor the core to a single store.
This solves a few issues: - The reset occurs synchronously with push, so in the same event loop tick where the internal state is updated: both states are invalidated at the same time - The queue doesn't build up over time - No more hacks in the pages router 🙌 We still need the auto-reset mechanism for the testing adapter, although I feel like we could probably get rid of that too. One thing at a time.
|
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* test: add 1099 reproduction in Next.js * test: add 1099 test for all frameworks * test: a state update in an effect is what seems to trigger the issue * fix: simplify the Next.js app router adapter * fix: only apply transition in shallow: false for react router On shallow: true, this caused the optimistic state to be updated after the queue has reset, and caused the 1099 issue where the state would be temporarily reset to the previous value by the hook's useEffect sync system. * test: add debug logs for 1099 * test: remove debug log (rr fix was flaky locally too) * fix(meh): don't clear the throttle queue (needs a better way) * fix: reset throttle queue before first push This solves a few issues: - The reset occurs synchronously with push, so in the same event loop tick where the internal state is updated: both states are invalidated at the same time - The queue doesn't build up over time - No more hacks in the pages router 🙌 We still need the auto-reset mechanism for the testing adapter, although I feel like we could probably get rid of that too. One thing at a time. * chore: restore StrictMode * test: restore the throw on NullDetector
2 participants
Adding tests to try and isolate the issue.
Validated:
Fixes #1099.