Conversation
🦋 Changeset detectedLatest commit: 4c130bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Size Change: +544 B (+0.53%) Total Size: 104 kB 📦 View Changed
ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark React
Details
| Benchmark suite | Current: 4c130bb | Previous: a76996a | Ratio |
|---|---|---|---|
data-client: getlist-100 |
121.22 ops/s (± 4.5%) |
125 ops/s (± 4.8%) |
1.03 |
data-client: getlist-500 |
39.61 ops/s (± 5.3%) |
39.84 ops/s (± 5.5%) |
1.01 |
data-client: update-entity |
298.57 ops/s (± 6.9%) |
312.5 ops/s (± 7.2%) |
1.05 |
data-client: update-user |
294.12 ops/s (± 8.6%) |
294.12 ops/s (± 7.8%) |
1 |
data-client: getlist-500-sorted |
41.67 ops/s (± 9.6%) |
43.39 ops/s (± 9.2%) |
1.04 |
data-client: update-entity-sorted |
263.16 ops/s (± 6.5%) |
263.16 ops/s (± 5.9%) |
1 |
data-client: update-entity-multi-view |
277.78 ops/s (± 8.2%) |
277.78 ops/s (± 7.0%) |
1 |
data-client: list-detail-switch-10 |
6.79 ops/s (± 8.1%) |
7.22 ops/s (± 5.2%) |
1.06 |
data-client: update-user-10000 |
69.44 ops/s (± 10.1%) |
71.43 ops/s (± 12.4%) |
1.03 |
data-client: invalidate-and-resolve |
32.26 ops/s (± 6.2%) |
32.95 ops/s (± 5.6%) |
1.02 |
data-client: unshift-item |
200 ops/s (± 4.8%) |
188.68 ops/s (± 5.9%) |
0.94 |
data-client: delete-item |
256.41 ops/s (± 4.5%) |
250 ops/s (± 4.4%) |
0.98 |
data-client: move-item |
162.61 ops/s (± 7.1%) |
158.77 ops/s (± 8.3%) |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 4c130bb | Previous: 93555f8 | Ratio |
|---|---|---|---|
normalizeLong |
577 ops/sec (±3.24%) |
447 ops/sec (±4.75%) |
0.77 |
normalizeLong Values |
491 ops/sec (±1.21%) |
408 ops/sec (±1.51%) |
0.83 |
normalizeLong Scalar |
443 ops/sec (±3.14%) |
352 ops/sec (±3.69%) |
0.79 |
normalizeLong Scalar update |
1114 ops/sec (±0.61%) |
895 ops/sec (±0.68%) |
0.80 |
denormalizeLong |
292 ops/sec (±6.58%) |
233 ops/sec (±6.00%) |
0.80 |
denormalizeLong Values |
276 ops/sec (±4.64%) |
213 ops/sec (±4.94%) |
0.77 |
denormalizeLong donotcache |
1350 ops/sec (±1.50%) |
1002 ops/sec (±0.64%) |
0.74 |
denormalizeLong Values donotcache |
959 ops/sec (±0.31%) |
737 ops/sec (±0.59%) |
0.77 |
denormalizeLong Scalar donotcache |
1493 ops/sec (±0.22%) |
1073 ops/sec (±0.13%) |
0.72 |
denormalizeShort donotcache 500x |
1709 ops/sec (±1.54%) |
1437 ops/sec (±0.29%) |
0.84 |
denormalizeShort 500x |
706 ops/sec (±5.91%) |
639 ops/sec (±6.97%) |
0.91 |
denormalizeShort 500x withCache |
9310 ops/sec (±0.08%) |
6834 ops/sec (±5.52%) |
0.73 |
queryShort 500x withCache |
4357 ops/sec (±0.74%) |
3206 ops/sec (±0.97%) |
0.74 |
buildQueryKey All |
65499 ops/sec (±2.05%) |
58478 ops/sec (±1.39%) |
0.89 |
query All withCache |
7240 ops/sec (±3.99%) |
5828 ops/sec (±2.46%) |
0.80 |
denormalizeLong with mixin Entity |
276 ops/sec (±6.56%) |
209 ops/sec (±7.50%) |
0.76 |
denormalizeLong withCache |
8819 ops/sec (±0.16%) |
7517 ops/sec (±0.32%) |
0.85 |
denormalizeLong withCache (Scalar churn) |
8713 ops/sec (±0.80%) |
7491 ops/sec (±0.24%) |
0.86 |
denormalizeLong Values withCache |
8290 ops/sec (±1.22%) |
5132 ops/sec (±1.60%) |
0.62 |
denormalizeLong Scalar withCache |
9638 ops/sec (±0.23%) |
7648 ops/sec (±0.98%) |
0.79 |
denormalizeLong Scalar update withCache |
7384 ops/sec (±0.46%) |
4074 ops/sec (±0.24%) |
0.55 |
denormalizeLong All withCache |
7850 ops/sec (±0.23%) |
6058 ops/sec (±0.18%) |
0.77 |
denormalizeLong Query-sorted withCache |
7851 ops/sec (±2.11%) |
6098 ops/sec (±1.48%) |
0.78 |
denormalizeLongAndShort withEntityCacheOnly |
2077 ops/sec (±0.26%) |
1748 ops/sec (±0.19%) |
0.84 |
denormalize bidirectional 50 |
5463 ops/sec (±7.19%) |
4498 ops/sec (±10.41%) |
0.82 |
denormalize bidirectional 50 donotcache |
58648 ops/sec (±0.17%) |
42385 ops/sec (±1.43%) |
0.72 |
getResponse |
6426 ops/sec (±4.48%) |
4418 ops/sec (±4.05%) |
0.69 |
getResponse (null) |
12947782 ops/sec (±0.31%) |
10236651 ops/sec (±0.70%) |
0.79 |
getResponse (clear cache) |
249 ops/sec (±8.68%) |
203 ops/sec (±7.07%) |
0.82 |
getSmallResponse |
4917 ops/sec (±0.82%) |
3543 ops/sec (±0.24%) |
0.72 |
getSmallInferredResponse |
3851 ops/sec (±1.04%) |
2852 ops/sec (±1.79%) |
0.74 |
getResponse Collection |
6967 ops/sec (±1.82%) |
4306 ops/sec (±4.05%) |
0.62 |
get Collection |
5264 ops/sec (±0.25%) |
2707 ops/sec (±0.19%) |
0.51 |
get Query-sorted |
7483 ops/sec (±2.92%) |
5052 ops/sec (±1.47%) |
0.68 |
setLong |
581 ops/sec (±0.17%) |
467 ops/sec (±0.60%) |
0.80 |
setLongWithMerge |
338 ops/sec (±0.14%) |
257 ops/sec (±0.45%) |
0.76 |
setLongWithSimpleMerge |
339 ops/sec (±1.09%) |
272 ops/sec (±0.80%) |
0.80 |
setSmallResponse 500x |
1138 ops/sec (±1.08%) |
926 ops/sec (±1.48%) |
0.81 |
This comment was automatically generated by workflow using github-action-benchmark.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: FOLLOW_UP_ONLY on #4097 @ b0f6e54
Architecture holds. The two halves are not redundant: useEnhancedReducer hold keeps React-bound SET_RESPONSE / handleSet / clear tied to a fiber that actually commits (so a discarded tree cannot complete next() and wipe settledByMeta), and NetworkManager.init() re-resolve republishes into the controller that did commit when a shared manager’s held queue died with the discarded fiber. Middleware-still-immediate is the right split. throttle-only noteSettled matches the only path that enters this.fetching. Mutating settled.controller before re-resolve makes a second init() idempotent. Module-level WeakMap is fine — keys are per-FetchingMeta and never shared across manager instances. Strict Mode: heldRef goes [] → null once; remount sees null and does not double-apply. Changeset listing the linked group + use-enhanced-reducer matches .changeset/config.json. Neither half looks over-engineered relative to the failure mode.
FOLLOW_UP
- Race suite fixed waits (
precommit-race.node-suite.tsx): replacewait(100)/wait(500)with condition-based waits (text containsvalue 5/error nope, orcallsstable). Fixed sleeps will flake under load; the suite already sets a 20s timeout as a smell. renderDataHook/ActController.resolve× hold: agreed this is a helper bug, not a reason to unwind the production fix.makeRenderDataClientwrapsresolvein synchronousact(), which can race the hold’s mountuseEffect(promise microtasks vs passive effects) so the hold never opens and suspense stays pending — especially with sync fixtures. Fix the test helper (flush/open hold reliably, or stop sync-act-wrappingresolve) in a follow-up souseFetch-use.web.tsxand siblings do not timeout. Do not block this PR on that unless CI is red on HEAD.
No CHANGE_THIS_PR.
Queue actions that reach React before the mount effect and replay them from that effect. Middleware still runs immediately, and the promise from dispatch still resolves once the action commits. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
A shared NetworkManager can resolve a fetch into a controller whose store never commits. On init, publish that result through the controller that did commit, without calling the endpoint again. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
… race suite Wait for the endpoint call before releasing the blocker, then for the expected text and a stable call count. Drops the 20s suite timeout. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
b0f6e54 to
8f0f5a9
Compare
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: RE_LGTM on #4097 @ 8f0f5a9
Prior FOLLOW_UP_ONLY @ b0f6e54 is closed:
- Race suite now uses condition waits (
waitUntil/waitUntilStable) instead ofwait(100)/wait(500)—8f0f5a9a. renderDataHook/ Kick harness is on base via #4099 rebase; not required in this PR.
Production useEnhancedReducer hold + NetworkManager noteSettled/init() are unchanged vs the prior review. Two-layer architecture still holds; no CHANGE_THIS_PR. Merge when required CI (incl. Bugbot) is green on this HEAD.
React 17 and 18 have no use(), and react-dom 17 has no react-dom/client. The blocker now suspends by throwing its promise when use() is missing, and the DOM host falls back to a legacy root on React 17. Each version asserts what it actually does before the blocker releases: concurrent roots (18, 19) show no value yet, and legacy roots (17) have already committed DataProvider, so the race cannot happen there. Transition cases skip on 17, and use(useFetch()) cases skip before 19. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Use the shared streamingHarness gate instead of a local flag plus resolver, load legacy react-dom with a typed require instead of a cast, and keep the case filter boolean. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: RE_LGTM on #4097 @ 6e0e572
Delta vs prior RE_LGTM @ 8f0f5a9 is test-only (255afaac, 6e0e572):
- Race suite now runs on React 17 and 18 with version-correct assertions: legacy roots expect the value before release (no race); concurrent roots expect it absent before release when a blocker is present (proves the park).
- Skips match reality (
startTransitionon 17,use(useFetch())before 19). - DOM host uses
ReactDOM.renderunderLegacyReact; blocker reusesmakeGatewith throw-for-17.
Production useEnhancedReducer hold + NetworkManager noteSettled/init() are byte-identical to 8f0f5a9. Two-layer architecture still holds. No CHANGE_THIS_PR; no new FOLLOW_UP. Merge when required CI (incl. Bugbot) is green on this HEAD.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6e0e572. Configure here.
| const element = | ||
| LegacyReact ? | ||
| <Suspense fallback={<Text>outer</Text>}>{tree}</Suspense> | ||
| : tree; |
There was a problem hiding this comment.
Outer Suspense hides provider on React 17
Medium Severity
The new LegacyReact wrapper puts DataProvider and Blocker in the same Suspense, so a pending sibling shows the outer fallback and the provider never commits before gate.release(). beforeRelease then reads that fallback instead of the resolved value, and the React 17 assertion fails for every blocked case, including the rejected-fetch test.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6e0e572. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4097 +/- ##
==========================================
+ Coverage 97.84% 97.86% +0.02%
==========================================
Files 156 156
Lines 3057 3086 +29
Branches 612 619 +7
==========================================
+ Hits 2991 3020 +29
Misses 18 18
Partials 48 48 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…-commit race suite Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…acy and concurrent roots Co-authored-by: Nathaniel Tucker <me@ntucker.me>
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: RE_LGTM on #4097 @ 4c130bb
Delta vs prior RE_LGTM @ 6e0e572 is test-only (b34f277, 4c130bb):
- Race suite pins fresh-manager call counts by root type (
shared || LegacyReact→ exactly one call; concurrent fresh managers still assert discard churn). - New
precommit-hold.tsxcovers the hold on legacy and concurrent roots (replay in one mount-effect render; concurrent batches promise-callback dispatches).
Production useEnhancedReducer hold + NetworkManager noteSettled/init() are byte-identical to 6e0e572. Two-layer architecture still holds. No CHANGE_THIS_PR; no new FOLLOW_UP.
Bugbot waive: the open "Outer Suspense hides provider on React 17" finding is a false positive. Same harness @ 6e0e572 had green ci/circleci: unit_tests-^17 (and ^18 / latest / 19.3 / native); ReactDOM collects precommit-race.tsx, so the LegacyReact beforeRelease assertions are already exercised and passing. Do not restructure the outer Suspense for that report.
Merge when required CI (incl. Bugbot re-run or this waive) is green on this HEAD.
This branch has not been deployed
2 participants
Motivation
useSuspenseanduse(useFetch())can stay on the fallback when a fetch finishes whileDataProviderhas not committed. That happens when the root render is parked on another thenable, such as a siblinguse(). React discards the uncommitted store, and a sharedNetworkManagerkeeps a promise that never settles. With a new manager per provider, the same race refetches in a loop.Holding the React dispatch until the store commits is not enough on its own. The held queue belongs to the discarded fiber. The shared manager still has to publish the result it already has into the store that does commit.
Solution
Two changes, neither of which touches
useSuspense:useEnhancedReducerqueues actions that reach React before its mount effect, then replays them in order. Middleware still runs immediately, so the endpoint is not delayed.dispatchstill resolves once the action commits.NetworkManagerrecords a throttled fetch's result and the controller it resolved into, including errors. When a store commits,init()re-resolves any result whose controller is not the one that committed, without calling the endpoint again.The same race was run with the act environment off on React DOM 19.2.3 (
createRoot) and on the React Native Jest runtime (react-native0.86.2,react-test-renderer). Hold-only left the shared-manager case on the fallback in both. Hold plus re-resolve renders the value, calls the endpoint once for a shared manager, and does not log "hasn't mounted yet". The two runtimes behave the same.The race suite waits on conditions (endpoint called, expected text rendered, call count stable) rather than fixed sleeps.
Rebased on master, which includes the
renderDataHookfix from #4099, sopackages/react/src/__tests__/useFetch-use.web.tsxpasses with the hold in place.React 17 and 18
The race suite runs on every React version CI tests, and each version asserts what it actually does:
createRoot().render()parks the whole root on a suspended sibling. On master it stays on the fallback (32 endpoint calls with fresh managers). With this change it renders the value, the same as 19. Before the blocker releases, the suite asserts that the value is not shown yet, which proves the render was parked. On 17 and 18 the blocker suspends by throwing its promise, becauseuse()exists only from 19. On 19 it still usesuse().DataProviderwhile a sibling is suspended, so the race cannot happen there. The suite asserts that the value is already shown before the blocker releases. The DOM host usesReactDOM.renderbecause react-dom 17 has noreact-dom/client.startTransition.use(useFetch())cases skip before 19.NetworkManager.The race suite without this change, run against master's
NetworkManageranduseEnhancedReducer: 17 passes (it has no race), 18 fails 4 tests, and 19 fails 7.packages/use-enhanced-reducer/src/__tests__/precommit-hold.tsxcovers the hold directly, on legacy roots (17, andReactDOM.renderon 18) and oncreateRoot(18, 19). A legacy root commits synchronously and runs the mount effect later, so the test checks that actions dispatched during render are held and then replayed in one render. Against master'suseEnhancedReducer, it fails only on legacy roots. It passes with this change on 17.0.2, 18.3.1, and 19.2.3.