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

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

eps1lon
Copy link
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)];
  ```
Copy link

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

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
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.

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.

None yet

3 participants