Skip to content

Current behavior of "What caused this update?" to point to host root after a ping#29735

Draft
eps1lon wants to merge 5 commits intofacebook:mainfrom
eps1lon:updaters-lazy-components
Draft

Current behavior of "What caused this update?" to point to host root after a ping#29735
eps1lon wants to merge 5 commits intofacebook:mainfrom
eps1lon:updaters-lazy-components

Conversation

@eps1lon
Copy link
Copy Markdown
Collaborator

@eps1lon eps1lon commented Jun 3, 2024

Based on #28330

It seems like we always use the updater that triggered the wakeable to be considered the updater on the ping. That is confusing IMO. The ping should be the updater. Some dirty fixes revealed other oddities with error boundaries.

Might revisit this in the future if we're still bought into updater tracking

Sebastian Silbermann and others added 5 commits June 2, 2024 20:39
Fixes
````
SyntaxError: ~/react/packages/react-reconciler/src/ReactFiberLane.js: Compiling let/const in this block would add a closure (throwIfClosureRequired).
  614 |   if (updateLane !== IdleLane) {
  615 |     if (enableUpdaterTracking) {
> 616 |       if (isDevToolsPresent) {
      |                              ^
  617 |         // transfer pending updaters from pingedLanes to updateLane
  618 |         const pendingUpdatersLaneMap = root.pendingUpdatersLaneMap;
  619 |         const updaters = pendingUpdatersLaneMap[laneToIndex(updateLane)];
  ```
@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 3:45pm

@react-sizebot
Copy link
Copy Markdown

Comparing: d77dd31...e457cb4

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 496.48 kB 496.45 kB = 88.87 kB 88.86 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 501.30 kB 501.27 kB = 89.56 kB 89.55 kB
facebook-www/ReactDOM-prod.classic.js = 593.97 kB 593.94 kB = 104.48 kB 104.48 kB
facebook-www/ReactDOM-prod.modern.js = 570.35 kB 570.33 kB = 100.89 kB 100.88 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Generated by 🚫 dangerJS against e457cb4

Comment on lines +448 to 450
// TODO: If this was a single commit, ErrorBoundary shouldn't be in here.
// We only set state in Parent so that should be the only updater
expect(allSchedulerTypes).toEqual([[Parent], [ErrorBoundary]]);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrorBoundary does make sense I believe. It's kind of like a rerender triggered from a child. Though just like the Fiber isn't generally sufficient information for setState updates, the error boundary isn't generally sufficient for "rerenders" due to errors. You'd want the boundary + the origin of the error.

@nidhishgajjar
Copy link
Copy Markdown

Orb Code Review (powered by GLM 5.1 on Orb Cloud)

Summary

This PR addresses how React DevTools' "What caused this update?" feature tracks updaters when Suspense boundaries are pinged (i.e., when awaited data resolves). Currently, after a ping triggers re-render, the updater tracking incorrectly points to the host root. The PR introduces a movePendingUpdatersToLane function to preserve updater identity across lane transitions, and documents the current (partially broken) behavior in tests.

Architecture

The changes are confined to ReactFiberLane.js (core lane management) and ReactUpdaters-test.internal.js (updater tracking tests). This fits cleanly into the existing updater tracking subsystem (enableUpdaterTracking / isDevToolsPresent guards). The key insight: when markRootUpdated clears pingedLanes, any updaters registered on those lanes are lost. The fix moves them to the new update lane before the clear.

Issues

packages/react-reconciler/src/ReactFiberLane.jsmovePendingUpdatersToLane edge case (warning)

The new movePendingUpdatersToLane function has a subtle edge case: if sourceLanes contains the same lane as targetLane, then sourceUpdaters and targetUpdaters reference the same Set object. The code adds elements to the target set (a no-op since they're the same set), then calls sourceUpdaters.clear() — which also clears targetUpdaters, losing all updaters for that lane.

const sourceUpdaters = pendingUpdatersLaneMap[index];
sourceUpdaters.forEach(sourceUpdater => {
  targetUpdaters.add(sourceUpdater);  // no-op if same Set
});
sourceUpdaters.clear();  // ALSO clears targetUpdaters if same Set!

In practice this may not trigger because updateLane (the target) is typically a freshly assigned lane distinct from pinged lanes, but a defensive guard would be safer:

if (index !== targetIndex) {
  sourceUpdaters.forEach(sourceUpdater => {
    targetUpdaters.add(sourceUpdater);
  });
  sourceUpdaters.clear();
}

packages/react-reconciler/src/ReactFiberLane.jsmarkRootPinged semantics change (suggestion)

The removal of root.suspendedLanes & from markRootPinged means pingedLanes can now contain lanes that aren't suspended:

// Before:
root.pingedLanes |= root.suspendedLanes & pingedLanes;
// After:
root.pingedLanes |= pingedLanes;

The TODO comment acknowledges this. This change is necessary for the updater tracking fix to work (since movePendingUpdatersToLane reads from pingedLanes before they're cleared). Consider updating the comment to explain why the & suspendedLanes guard was removed — namely that markRootUpdated now consumes pingedLanes before clearing them, so the guard would prevent updaters from being moved.

Test file — good improvements (positive note)

  • Converting from legacy ReactDOM.render to createRoot aligns with React's direction
  • Splitting the ping test into "after update" and "after mount" cases provides better coverage
  • The "after mount" test correctly documents the known broken behavior ([null] instead of [Suspender]) rather than silently passing
  • Using .length = 0 over .splice(0) is a minor but welcome cleanup

Cross-file impact

No cross-file concerns. The movePendingUpdatersToLane function is module-private and only called from markRootUpdated. The markRootPinged change affects all callers, but since markRootUpdated is always called when processing updates (and it clears pingedLanes), downstream consumers should be unaffected.

Assessment

comment — This is a diagnostic PR that documents and partially improves updater tracking around Suspense pings. The code is reasonable and the test improvements are solid. The edge case in movePendingUpdatersToLane is worth a defensive guard but unlikely to cause issues in practice given how lane assignment works. The PR clearly communicates its scope (documenting current behavior, not fully fixing it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants