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.

Sign up for GitHub

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

Jump to bottom

fix[devtools/updateFiberRecursively]: mount suspense fallback set in timed out case #27147

Merged
merged 1 commit into from
Aug 3, 2023
Merged

fix[devtools/updateFiberRecursively]: mount suspense fallback set in timed out case #27147

merged 1 commit into from
Aug 3, 2023

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Jul 25, 2023

Fixes #26793.

I have received a constantly reproducible example of the error, that is mentioned in the issue above.
When starting Reload and Profile in DevTools, React reports an unmount of a functional component inside Suspense's fallback via onCommitFiberUnmount in commitDeletionEffectsOnFiber, but this fiber was never registered as mounted in DevTools.

While debugging, I've noticed that in timed-out case for Suspense trees we only check if both previous fallback child set and next fiber fallback child set are non-null, but in these recursive calls there is also a case when previous fallback child set is null and next set is non-null, so we were skipping the branch.

Screenshot 2023-07-25 at 15 26 07

After these changes, the issue is no longer reproducible, but I am not sure if this is the right solution, since I don't know if this case is correct from reconciler perspective.

@hoxyq hoxyq requested review from gaearon and acdlite July 25, 2023 14:30
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 25, 2023
@hoxyq hoxyq force-pushed the devtools/mount-suspense-fallback-set-in-timed-out-case branch 2 times, most recently from 3d94214 to 8bee920 Compare July 25, 2023 14:43
@hoxyq hoxyq force-pushed the devtools/mount-suspense-fallback-set-in-timed-out-case branch from 8bee920 to 107cdf5 Compare July 25, 2023 14:43
@hoxyq hoxyq requested a review from sebmarkbage August 1, 2023 10:23
acdlite
acdlite approved these changes Aug 3, 2023
View reviewed changes
Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with this code but the fix makes sense to me based on your description

@hoxyq hoxyq merged commit 997f52f into facebook:main Aug 3, 2023
2 checks passed
@hoxyq hoxyq deleted the devtools/mount-suspense-fallback-set-in-timed-out-case branch August 3, 2023 19:02
@gnoff gnoff mentioned this pull request Aug 8, 2023
Merged
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Aug 8, 2023
@gnoff
update vendored React packages to the the latest canary (#53742)
f0dab3a 
Updated React from 9377e1010 to cb3404a0c.

### React upstream changes

- facebook/react#27190
- facebook/react#27189
- facebook/react#27201
- facebook/react#27147
- facebook/react#26949
- facebook/react#27058
- facebook/react#27142
- facebook/react#27133
- facebook/react#27130
- facebook/react#27105
- facebook/react#27117
- facebook/react#27057
@hoxyq hoxyq mentioned this pull request Aug 29, 2023
Merged
hoxyq added a commit that referenced this pull request Aug 29, 2023
@hoxyq
React DevTools 4.28.0 -> 4.28.1 (#27305)
2f36872 
List of changes:
* refactor: refactored devtools browser extension scripts to improve
port management and service worker lifetime
([hoxyq](https://github.com/hoxyq) in
[#27215](#27215))
* refactor[devtools/extension]: minify production builds to strip
comments ([hoxyq](https://github.com/hoxyq) in
[#27304](#27304))
* fix[devtools]: allow element updates polling only if bridge is alive
([hoxyq](https://github.com/hoxyq) in
[#27067](#27067))
* refactor: resolve browser via env variables based on build rather than
user agent ([hoxyq](https://github.com/hoxyq) in
[#27179](#27179))
* fix[devtools/updateFiberRecursively]: mount suspense fallback set in
timed out case ([hoxyq](https://github.com/hoxyq) in
[#27147](#27147))
* Feat:-Added open in editor to appear by default
([Biki-das](https://github.com/Biki-das) in
[#26949](#26949))
* fix[devtools/inspect]: null check memoized props before trying to call
hasOwnProperty ([hoxyq](https://github.com/hoxyq) in
[#27057](#27057))
* rename SuspenseList export to unstable_SuspenseList
([noahlemen](https://github.com/noahlemen) in
[#27061](#27061))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@hoxyq @AndyPengc12
fix[devtools/updateFiberRecursively]: mount suspense fallback set in …
d8b20a9 
…timed out case (facebook#27147)

Fixes facebook#26793.

I have received a constantly reproducible example of the error, that is
mentioned in the issue above.
When starting `Reload and Profile` in DevTools, React reports an unmount
of a functional component inside Suspense's fallback via
[`onCommitFiberUnmount`](https://github.com/facebook/react/blob/3ff846d106de9273f59d1e4457793a5fcf625aef/packages/react-devtools-shared/src/hook.js#L408-L413)
in
[`commitDeletionEffectsOnFiber`](https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberCommitWork.js#L2025),
but this fiber was never registered as mounted in DevTools.

While debugging, I've noticed that in timed-out case for Suspense trees
we only check if both previous fallback child set and next fiber
fallback child set are non-null, but in these recursive calls there is
also a case when previous fallback child set is null and next set is
non-null, so we were skipping the branch.

<img width="1746" alt="Screenshot 2023-07-25 at 15 26 07"
src="https://github.com/facebook/react/assets/28902667/da21a682-9973-43ec-9653-254ba98a0a3f">

After these changes, the issue is no longer reproducible, but I am not
sure if this is the right solution, since I don't know if this case is
correct from reconciler perspective.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@hoxyq @AndyPengc12
React DevTools 4.28.0 -> 4.28.1 (facebook#27305)
7a25b2d 
List of changes:
* refactor: refactored devtools browser extension scripts to improve
port management and service worker lifetime
([hoxyq](https://github.com/hoxyq) in
[facebook#27215](facebook#27215))
* refactor[devtools/extension]: minify production builds to strip
comments ([hoxyq](https://github.com/hoxyq) in
[facebook#27304](facebook#27304))
* fix[devtools]: allow element updates polling only if bridge is alive
([hoxyq](https://github.com/hoxyq) in
[facebook#27067](facebook#27067))
* refactor: resolve browser via env variables based on build rather than
user agent ([hoxyq](https://github.com/hoxyq) in
[facebook#27179](facebook#27179))
* fix[devtools/updateFiberRecursively]: mount suspense fallback set in
timed out case ([hoxyq](https://github.com/hoxyq) in
[facebook#27147](facebook#27147))
* Feat:-Added open in editor to appear by default
([Biki-das](https://github.com/Biki-das) in
[facebook#26949](facebook#26949))
* fix[devtools/inspect]: null check memoized props before trying to call
hasOwnProperty ([hoxyq](https://github.com/hoxyq) in
[facebook#27057](facebook#27057))
* rename SuspenseList export to unstable_SuspenseList
([noahlemen](https://github.com/noahlemen) in
[facebook#27061](facebook#27061))
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
@hoxyq
fix[devtools/updateFiberRecursively]: mount suspense fallback set in …
7c1d843 
…timed out case (#27147)

Fixes #26793.

I have received a constantly reproducible example of the error, that is
mentioned in the issue above.
When starting `Reload and Profile` in DevTools, React reports an unmount
of a functional component inside Suspense's fallback via
[`onCommitFiberUnmount`](https://github.com/facebook/react/blob/3ff846d106de9273f59d1e4457793a5fcf625aef/packages/react-devtools-shared/src/hook.js#L408-L413)
in
[`commitDeletionEffectsOnFiber`](https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberCommitWork.js#L2025),
but this fiber was never registered as mounted in DevTools.

While debugging, I've noticed that in timed-out case for Suspense trees
we only check if both previous fallback child set and next fiber
fallback child set are non-null, but in these recursive calls there is
also a case when previous fallback child set is null and next set is
non-null, so we were skipping the branch.

<img width="1746" alt="Screenshot 2023-07-25 at 15 26 07"
src="https://github.com/facebook/react/assets/28902667/da21a682-9973-43ec-9653-254ba98a0a3f">

After these changes, the issue is no longer reproducible, but I am not
sure if this is the right solution, since I don't know if this case is
correct from reconciler perspective.

DiffTrain build for commit 997f52f.
@hoxyq hoxyq mentioned this pull request Jul 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acdlite acdlite acdlite approved these changes

@gaearon gaearon Awaiting requested review from gaearon

@sebmarkbage sebmarkbage Awaiting requested review from sebmarkbage

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DevTools Bug] Cannot remove node "226752" because no matching node was found in the Store.
3 participants
@hoxyq @acdlite @facebook-github-bot