fix(react-context-selector): avoid useState eager-bailout pitfall#36002
Open
layershifter wants to merge 4 commits intomasterfrom
Open
fix(react-context-selector): avoid useState eager-bailout pitfall#36002layershifter wants to merge 4 commits intomasterfrom
layershifter wants to merge 4 commits intomasterfrom
Conversation
…r-bailout pitfall
The previous implementation depends on React's useState eager-bailout fast
path in dispatchSetStateInternal:
if (fiber.lanes === NoLanes && (alternate === null || alternate.lanes === NoLanes)) {
// eager bailout — drop the update without scheduling a render
}
The fiber passed is the one bound at mount. After the first listener-driven
render commits, that fiber becomes the alternate of the new current — but its
.lanes is never cleared from the enqueueUpdate that preceded the render.
From that point on, every subsequent listener dispatch fails the NoLanes
precondition. Updates are queued normally, schedule a render, and the reducer
runs in-render only to return prevState. React then calls
bailoutOnAlreadyFinishedWork — the DOM doesn't update, but the component
function already executed. This is the "Glitchy Behavior" documented in the
context-selector-tearing RFC.
New implementation:
- Reads valueRef.current during render (like useSyncExternalStore's getSnapshot).
- No [value, selected] state tuple. No setState(prev => prev) trick.
- useReducer(x => x + 1) as opaque force-update counter.
- Listener compares selector(newValue) against lastReturnedRef and forces only
when the slice actually differs.
- Layout-effect fixup per Relay's useFragmentInternal pattern.
Preserves: public API, listener payload shape ([version, value]),
useHasParentContext behavior, Level 1 tearing behavior.
See RFC PR for the full Option D writeup.
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
…ive-deps The react-compiler rule errors when react-hooks rules are disabled, and exhaustive-deps doesn't actually complain about the effect's deps (both listeners and valueRef are referentially stable context fields).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ssion tests Addressing code review feedback on #36002: 1. [BLOCKER] Restore try/catch around selector calls in both the listener and the effect-fixup. The previous implementation wrapped the selector in a try/catch with the comment 'ignored (stale props or some other reason)'. The new hook dropped that guard; since the provider iterates listeners with Array.prototype.forEach which aborts on throw, any consumer whose selector throws on an intermediate payload would silently starve all later subscribers of that update. Restoring the guard preserves the prior isolation contract. 2. Add an inline comment explaining why the new hook destructures only { value, listeners } from the context and ignores the version field. The version is still maintained by the provider and consulted by useHasParentContext as a parent-presence sentinel; it is no longer needed as a staleness guard because the provider fires listeners synchronously inside useIsomorphicLayoutEffect, and freshness is guaranteed by the effect-fixup. 3. Add regression tests: - 'memoized consumers re-render only when their selected slice changes': the RFC's Scenario 2 with React.memo'd items, asserting per-item onUpdate call counts across a cycling sequence. Catches the 'three items re-rendered instead of two' glitch if it regresses. - 'a single consumer throw does not starve later subscribers of context updates': exercises the listener error-isolation path restored above. StrictMode sanity-checked locally: layout-effect mount → cleanup → mount in dev produces idempotent ref re-assignments and no extra forceUpdate() calls — render counts scale by React's dev-mode 2x factor as expected, with no pathological churn.
layershifter
force-pushed
the
layershifter/fix-context-selector-bailout
branch
from
8eed133 to
3cc3c8a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rewrites
@fluentui/react-context-selector'suseContextSelectorhook so it no longer depends on React'suseState- eager-bailout fast path. The previous implementation works correctly for the first listener-driven render of any memoized consumer, but silently degrades for every subsequent one - producing wasted render passes that React later discards viabailoutOnAlreadyFinishedWork.Why
See a complete explainer in #36001.
The fix
The new hook avoids the eager-bailout dependency entirely:
selector(valueRef.current)directly during render (same asuseSyncExternalStore'sgetSnapshot— the only read ofvalueRef.currentduring render).useReducer(x => x + 1)as an opaque force-update counter. The reducer queue is never walked by the eager-bailout codepath because it's invoked throughdispatch()with no action.selector(payload)againstlastReturnedRef.currentand callsforceUpdate()only when the selected slice actually changed. NosetState(prev => prev)trick is used, sofiber.lanesis never polluted with bailed-out updates.useFragmentInternal) catches updates that occurred between render and effect run.Tearing behavior is unchanged from main — still Level 1 per the original RFC's accepted tradeoff.
Measurements
React 18.3.1, StrictMode off. Render counts per item over a fixed click sequence.
Memoized ListItems (matches RFC "Scenario 2")
set 1 (4→1), item 1 rendersset 2 (1→2)2nd, item 2set 3 (2→3)2nd, item 3Eliminates the "+2 on 2nd activation" anomaly the RFC flagged as "some kind of issue with the
useState()bailout mechanism".Plain (non-memoized) ListItems
set 1 (4→1), any itemNo regression. Both main and this PR re-render plain items once from parent cascade and again from listener-driven heal (Level 1 tearing behavior retained).
Parent tick (Provider value unchanged)
Identical. Provider-value-unchanged re-renders skip memoized descendants regardless.
Tearing (parent-prop-vs-context)
Level 1 tearing preserved — explicit non-goal per the original RFC.
Not covered