Skip to content

/ react Public

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

Jump to bottom

[DevTools] Update Fiber logic in backend renderer to match implementation in React #22527

Merged
merged 2 commits into from Oct 8, 2021
Merged

[DevTools] Update Fiber logic in backend renderer to match implementation in React #22527

merged 2 commits into from Oct 8, 2021
+40 −54

Conversation

@jstejada
Copy link
Contributor

@jstejada jstejada commented Oct 8, 2021 •
edited

Summary

There is an issue reported internally where every time a specific component is inspected, we get the following error in the backend:

Unable to find node on an unmounted component..

It appears to be happening because we are incorrectly detecting the component to be unmounted. As specified in this comment, I noticed that our implementation wasn't actually matching the one in React, so I updated it, and that seemed to solve the issue.

How did you test this change?

  • yarn flow
  • yarn test
  • yarn test-build-devtools
  • error no longer occurs in internal repro case after applying these changes
  • regression check on older react version:
@jstejada
[DevTools] Update isMountedImpl to match implementation in React
c2abf11
@jstejada jstejada requested review from bvaughn and lunaruan Oct 8, 2021
@jstejada jstejada marked this pull request as draft Oct 8, 2021
bvaughn
bvaughn reviewed Oct 8, 2021
View changes
Copy link
Contributor

@bvaughn bvaughn left a comment

This change looks substantial. I trust that it fixes the is-mounted-logic for the latest React version, but I wonder if it will break any older versions (like the one we originally forked from).

Do we know which commit to ReactFiberTreeReflection broke our mirror function here? Can we do a regression check for earlier versions of React to make sure we don't break them with this change?

packages/react-devtools-shared/src/backend/renderer.js Outdated Show resolved Hide resolved
@jstejada
Copy link
Contributor Author

@jstejada jstejada commented Oct 8, 2021

Do we know which commit to ReactFiberTreeReflection broke our mirror function here? Can we do a regression check for earlier versions of React to make sure we don't break them with this change?

@bvaughn that makes sense, i'll look into it. I took a quick glance at the blame of ReactFiberTreeReflection and most commits touching that logic are pretty old, like 2+ years, so i'll have to check when exactly the DevTools version looked like that one

@jstejada
Copy link
Contributor Author

@jstejada jstejada commented Oct 8, 2021 •
edited

@bvaughn, alright, looks like the implementation changed 2 years ago in this commit: 8d7c733#diff-d80a1a14b2c962c703749c53ad26571becc96aea7e21e6af0cfdbfa652dd80a4

The current DevTools implementation was added 3 years ago, here: b56bc1a (which matched this implementation of roughly 3 years ago as well: ReactFiberTreeReflection.js ~ 3 years ago).

In terms of doing a regression check, the procedure would be to build a version of React from before 8d7c733 and verify that DevTools still works correctly against that one?

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 8, 2021

Sounds right!

@jstejada jstejada changed the title [DevTools] Update isMountedImpl to match implementation in React [DevTools] Update Fiber logic in backend renderer to match implementation in React Oct 8, 2021
@jstejada
Copy link
Contributor Author

@jstejada jstejada commented Oct 8, 2021

as we discussed offline, tested older version of react against CodeSandbox using latest DevTools, seems to work corectly (i updated the test plan for the PR with details)

@jstejada
Also sync findCurrentFiberUsingSlowPathById
75e3f24
@jstejada jstejada marked this pull request as ready for review Oct 8, 2021
bvaughn
bvaughn approved these changes Oct 8, 2021
View changes
@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 8, 2021

Thanks for testing!

@jstejada jstejada merged commit afcb9cd into facebook:main Oct 8, 2021
33 checks passed
@jstejada jstejada deleted the fix-detecting-unmounted-fiber branch Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bvaughn bvaughn

@crypt096 crypt096

@lunaruan lunaruan

Assignees
No one assigned
Labels
Component: Developer Tools
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@jstejada @bvaughn @crypt096