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

fix[devtools]: check if fiber is unmounted before trying to highlight #26983

Merged

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Jun 20, 2023

For React Native environment, we sometimes spam the console with warnings "Could not find Fiber with id ...".

This is an attempt to fix this or at least reduce the amount of such potential warnings being thrown.

Now checking if fiber is already unnmounted before trying to get native nodes for fiber. This might happen if you try to inspect an element in DevTools, but at the time when event has been received, the element was already unmounted.

@hoxyq hoxyq requested a review from tyao1 June 20, 2023 18:45
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 20, 2023
@@ -104,15 +104,21 @@ export default function setupHighlighter(
const renderer = agent.rendererInterfaces[rendererID];
if (renderer == null) {

Choose a reason for hiding this comment

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

Why == and not ===?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To check if a value isn't equal to undefined or null with a single statement

@hoxyq hoxyq marked this pull request as ready for review June 21, 2023 11:19
Comment on lines -2743 to -2753
// Special case for a timed-out Suspense.
const isTimedOutSuspense =
fiber.tag === SuspenseComponent && fiber.memoizedState !== null;
if (isTimedOutSuspense) {
// A timed-out Suspense's findDOMNode is useless.
// Try our best to find the fallback directly.
const maybeFallbackFiber = fiber.child && fiber.child.sibling;
if (maybeFallbackFiber != null) {
fiber = maybeFallbackFiber;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed, because we return hostFibers and this branch doesn't have any side effects

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find!

Comment on lines -2743 to -2753
// Special case for a timed-out Suspense.
const isTimedOutSuspense =
fiber.tag === SuspenseComponent && fiber.memoizedState !== null;
if (isTimedOutSuspense) {
// A timed-out Suspense's findDOMNode is useless.
// Try our best to find the fallback directly.
const maybeFallbackFiber = fiber.child && fiber.child.sibling;
if (maybeFallbackFiber != null) {
fiber = maybeFallbackFiber;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find!

@hoxyq hoxyq merged commit 794b770 into facebook:main Jun 22, 2023
@hoxyq hoxyq deleted the devtools/fix-highlighting-unmounted-fiber branch June 22, 2023 07:51
hoxyq added a commit that referenced this pull request Jul 4, 2023
List of changes:
* Devtools:-Removed unused CSS ([Biki-das](https://github.com/Biki-das)
in [#27032](#27032))
* fix[devtools/profilingCache-test]: specify correct version gate for
test ([hoxyq](https://github.com/hoxyq) in
[#27008](#27008))
* fix[devtools/ci]: fixed incorrect condition calculation for
@reactVersion annotation ([hoxyq](https://github.com/hoxyq) in
[#26997](#26997))
* fix[ci]: fixed jest configuration not to skip too many devtools tests
([hoxyq](https://github.com/hoxyq) in
[#26955](#26955))
* fix[devtools/standalone]: update webpack configurations
([hoxyq](https://github.com/hoxyq) in
[#26963](#26963))
* fix[devtools]: check if fiber is unmounted before trying to highlight
([hoxyq](https://github.com/hoxyq) in
[#26983](#26983))
* feat[devtools]: support x_google_ignoreList source maps extension
([hoxyq](https://github.com/hoxyq) in
[#26951](#26951))
* chore[devtools]: upgrade to webpack v5
([hoxyq](https://github.com/hoxyq) in
[#26887](#26887))
* fix[devtools]: display NaN as string in values
([hoxyq](https://github.com/hoxyq) in
[#26947](#26947))
* fix: devtools cannot be closed correctly
([Jack-Works](https://github.com/Jack-Works) in
[#25510](#25510))
* Fix:- Fixed dev tools inspect mode on Shadow dom
([Biki-das](https://github.com/Biki-das) in
[#26888](#26888))
* Updated copyright text to Copyright (c) Meta Platforms, Inc. and its …
([denmo530](https://github.com/denmo530) in
[#26830](#26830))
* Fix strict mode badge URL ([ibrahemid](https://github.com/ibrahemid)
in [#26825](#26825))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…facebook#26983)

For React Native environment, we sometimes spam the console with
warnings `"Could not find Fiber with id ..."`.

This is an attempt to fix this or at least reduce the amount of such
potential warnings being thrown.

Now checking if fiber is already unnmounted before trying to get native
nodes for fiber. This might happen if you try to inspect an element in
DevTools, but at the time when event has been received, the element was
already unmounted.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
List of changes:
* Devtools:-Removed unused CSS ([Biki-das](https://github.com/Biki-das)
in [facebook#27032](facebook#27032))
* fix[devtools/profilingCache-test]: specify correct version gate for
test ([hoxyq](https://github.com/hoxyq) in
[facebook#27008](facebook#27008))
* fix[devtools/ci]: fixed incorrect condition calculation for
@reactVersion annotation ([hoxyq](https://github.com/hoxyq) in
[facebook#26997](facebook#26997))
* fix[ci]: fixed jest configuration not to skip too many devtools tests
([hoxyq](https://github.com/hoxyq) in
[facebook#26955](facebook#26955))
* fix[devtools/standalone]: update webpack configurations
([hoxyq](https://github.com/hoxyq) in
[facebook#26963](facebook#26963))
* fix[devtools]: check if fiber is unmounted before trying to highlight
([hoxyq](https://github.com/hoxyq) in
[facebook#26983](facebook#26983))
* feat[devtools]: support x_google_ignoreList source maps extension
([hoxyq](https://github.com/hoxyq) in
[facebook#26951](facebook#26951))
* chore[devtools]: upgrade to webpack v5
([hoxyq](https://github.com/hoxyq) in
[facebook#26887](facebook#26887))
* fix[devtools]: display NaN as string in values
([hoxyq](https://github.com/hoxyq) in
[facebook#26947](facebook#26947))
* fix: devtools cannot be closed correctly
([Jack-Works](https://github.com/Jack-Works) in
[facebook#25510](facebook#25510))
* Fix:- Fixed dev tools inspect mode on Shadow dom
([Biki-das](https://github.com/Biki-das) in
[facebook#26888](facebook#26888))
* Updated copyright text to Copyright (c) Meta Platforms, Inc. and its …
([denmo530](https://github.com/denmo530) in
[facebook#26830](facebook#26830))
* Fix strict mode badge URL ([ibrahemid](https://github.com/ibrahemid)
in [facebook#26825](facebook#26825))
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…#26983)

For React Native environment, we sometimes spam the console with
warnings `"Could not find Fiber with id ..."`.

This is an attempt to fix this or at least reduce the amount of such
potential warnings being thrown.

Now checking if fiber is already unnmounted before trying to get native
nodes for fiber. This might happen if you try to inspect an element in
DevTools, but at the time when event has been received, the element was
already unmounted.

DiffTrain build for commit 794b770.
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.

4 participants