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

DevTools: Inline references to fiber flags #26542

Merged
merged 1 commit into from Apr 4, 2023
Merged

DevTools: Inline references to fiber flags #26542

merged 1 commit into from Apr 4, 2023

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 3, 2023

We shouldn't be referencing internal fields like fiber's flag directly of DevTools. It's an implementation detail. However, over the years a few of these have snuck in. Because of how DevTools is currently shipped, where it's expected to be backwards compatible with older versions of React, this prevents us from refactoring those fields inside the reconciler.

The plan we have to address this is to fix how DevTools is shipped: DevTools will be released in lockstep with each version of React.

Until then, though, I need a temporary solution because it's blocking a feature I'm working on. So in meantime, I'm going to have to fork the DevTool's code based on the React version, like we already do with the fiber TypeOfWork enum.

As a first step, I've inlined all the references to fiber flags into the specific call sites where they are used. Eventually we'll import these functions from the reconciler so they stay in sync, rather than maintaining duplicate copies of the logic.

@acdlite
DevTools: Inline references to fiber flags
2d709e2 
We shouldn't be referencing internal fields like fiber's `flag` directly
of DevTools. It's an implementation detail. However, over the years a
few of these have snuck in. Because of how DevTools is currently
shipped, where it's expected to be backwards compatible with older
versions of React, this prevents us from refactoring those fields inside
the reconciler.

The plan we have to address this is to fix how DevTools is shipped:
DevTools will be released in lockstep with each version of React.

Until then, though, I need a temporary solution because it's blocking a
feature I'm working on. So in meantime, I'm going to have to fork the
DevTool's code based on the React version, like we already do with the
fiber TypeOfWork enum.

As a first step, I've inlined all the references to fiber flags into
the specific call sites where they are used. Eventually we'll import
these functions from the reconciler so they stay in sync, rather than
maintaining duplicate copies of the logic.
@acdlite acdlite requested a review from mondaychen April 3, 2023 17:12
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 3, 2023
@mondaychen
Copy link
Contributor

mondaychen commented Apr 3, 2023 •
edited

Would it be better if all of these flags are defined in the same place at the top of this file?

  1. they are easier to maintain in case we had to keep them longer than expected
  2. we won't miss any of them when we move those functions to reconciler

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 3, 2023

I inlined them intentionally because even the concept of a flag shouldn't have leaked into DevTools. Ideally we wouldn't even leak the concept of a Fiber. DevTools should be interacting with higher level concepts like "is this component in an error state?" and "did this component perform any work during the last render?", rather than leaking internal types.

It's bad that these leaked in the first place but I'd rather confine them to the actual place where they are referenced instead of encouraging these references to spread even more.

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 3, 2023 •
edited

In other words, rather than fork the flag itself (e.g. Placement), we should be forking the implementation of the function they are used in (e.g. getNearestMountedFiber).

Then once the DevTools packaging is fixed, the final step is to import getNearestMountedFiber directly from React, rather than maintain a duplicate copy.

@mondaychen
Copy link
Contributor

I see, that makes sense!

@acdlite acdlite merged commit 0ba4d7b into facebook:main Apr 4, 2023
36 checks passed
@hoxyq hoxyq mentioned this pull request Apr 17, 2023
Merged
hoxyq added a commit that referenced this pull request Apr 17, 2023
@hoxyq
React DevTools 4.27.4 -> 4.27.5 (#26637)
77d3b02 
Full list of changes (not everything included in changelog):
* refactor[devtools]: copy to clipboard only on frontend side
([hoxyq](https://github.com/hoxyq) in
[#26604](#26604))
* Provide icon to edge devtools.
([harrygz889](https://github.com/harrygz889) in
[#26543](#26543))
* [BE] move shared types & constants to consolidated locations
([mondaychen](https://github.com/mondaychen) in
[#26572](#26572))
* remove backend dependency from the global hook
([mondaychen](https://github.com/mondaychen) in
[#26563](#26563))
* Replace deprecated `new-window` with
`webContents.setWindowOpenHandler()`
([Willie-Boy](https://github.com/Willie-Boy) in
[#26559](#26559))
* DevTools: Inline references to fiber flags
([acdlite](https://github.com/acdlite) in
[#26542](#26542))
* refactor[devtools]: forbid editing class instances in props
([hoxyq](https://github.com/hoxyq) in
[#26522](#26522))
* Move update scheduling to microtask
([acdlite](https://github.com/acdlite) in
[#26512](#26512))
* Remove unnecessary CIRCLE_CI_API_TOKEN checks
([mondaychen](https://github.com/mondaychen) in
[#26499](#26499))
* browser extension: improve script injection logic
([mondaychen](https://github.com/mondaychen) in
[#26492](#26492))
* [flow] make Flow suppressions explicit on the error
([kassens](https://github.com/kassens) in
[#26487](#26487))
kassens pushed a commit to kassens/react that referenced this pull request Apr 17, 2023
@acdlite @kassens
DevTools: Inline references to fiber flags (facebook#26542)
4079ccc 
We shouldn't be referencing internal fields like fiber's `flag` directly
of DevTools. It's an implementation detail. However, over the years a
few of these have snuck in. Because of how DevTools is currently
shipped, where it's expected to be backwards compatible with older
versions of React, this prevents us from refactoring those fields inside
the reconciler.

The plan we have to address this is to fix how DevTools is shipped:
DevTools will be released in lockstep with each version of React.

Until then, though, I need a temporary solution because it's blocking a
feature I'm working on. So in meantime, I'm going to have to fork the
DevTool's code based on the React version, like we already do with the
fiber TypeOfWork enum.

As a first step, I've inlined all the references to fiber flags into the
specific call sites where they are used. Eventually we'll import these
functions from the reconciler so they stay in sync, rather than
maintaining duplicate copies of the logic.
kassens pushed a commit to kassens/react that referenced this pull request Apr 17, 2023
@hoxyq @kassens
React DevTools 4.27.4 -> 4.27.5 (facebook#26637)
253e889 
Full list of changes (not everything included in changelog):
* refactor[devtools]: copy to clipboard only on frontend side
([hoxyq](https://github.com/hoxyq) in
[facebook#26604](facebook#26604))
* Provide icon to edge devtools.
([harrygz889](https://github.com/harrygz889) in
[facebook#26543](facebook#26543))
* [BE] move shared types & constants to consolidated locations
([mondaychen](https://github.com/mondaychen) in
[facebook#26572](facebook#26572))
* remove backend dependency from the global hook
([mondaychen](https://github.com/mondaychen) in
[facebook#26563](facebook#26563))
* Replace deprecated `new-window` with
`webContents.setWindowOpenHandler()`
([Willie-Boy](https://github.com/Willie-Boy) in
[facebook#26559](facebook#26559))
* DevTools: Inline references to fiber flags
([acdlite](https://github.com/acdlite) in
[facebook#26542](facebook#26542))
* refactor[devtools]: forbid editing class instances in props
([hoxyq](https://github.com/hoxyq) in
[facebook#26522](facebook#26522))
* Move update scheduling to microtask
([acdlite](https://github.com/acdlite) in
[facebook#26512](facebook#26512))
* Remove unnecessary CIRCLE_CI_API_TOKEN checks
([mondaychen](https://github.com/mondaychen) in
[facebook#26499](facebook#26499))
* browser extension: improve script injection logic
([mondaychen](https://github.com/mondaychen) in
[facebook#26492](facebook#26492))
* [flow] make Flow suppressions explicit on the error
([kassens](https://github.com/kassens) in
[facebook#26487](facebook#26487))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@acdlite @AndyPengc12
DevTools: Inline references to fiber flags (facebook#26542)
c8ec58d 
We shouldn't be referencing internal fields like fiber's `flag` directly
of DevTools. It's an implementation detail. However, over the years a
few of these have snuck in. Because of how DevTools is currently
shipped, where it's expected to be backwards compatible with older
versions of React, this prevents us from refactoring those fields inside
the reconciler.

The plan we have to address this is to fix how DevTools is shipped:
DevTools will be released in lockstep with each version of React.

Until then, though, I need a temporary solution because it's blocking a
feature I'm working on. So in meantime, I'm going to have to fork the
DevTool's code based on the React version, like we already do with the
fiber TypeOfWork enum.

As a first step, I've inlined all the references to fiber flags into the
specific call sites where they are used. Eventually we'll import these
functions from the reconciler so they stay in sync, rather than
maintaining duplicate copies of the logic.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@hoxyq @AndyPengc12
React DevTools 4.27.4 -> 4.27.5 (facebook#26637)
62637a7 
Full list of changes (not everything included in changelog):
* refactor[devtools]: copy to clipboard only on frontend side
([hoxyq](https://github.com/hoxyq) in
[facebook#26604](facebook#26604))
* Provide icon to edge devtools.
([harrygz889](https://github.com/harrygz889) in
[facebook#26543](facebook#26543))
* [BE] move shared types & constants to consolidated locations
([mondaychen](https://github.com/mondaychen) in
[facebook#26572](facebook#26572))
* remove backend dependency from the global hook
([mondaychen](https://github.com/mondaychen) in
[facebook#26563](facebook#26563))
* Replace deprecated `new-window` with
`webContents.setWindowOpenHandler()`
([Willie-Boy](https://github.com/Willie-Boy) in
[facebook#26559](facebook#26559))
* DevTools: Inline references to fiber flags
([acdlite](https://github.com/acdlite) in
[facebook#26542](facebook#26542))
* refactor[devtools]: forbid editing class instances in props
([hoxyq](https://github.com/hoxyq) in
[facebook#26522](facebook#26522))
* Move update scheduling to microtask
([acdlite](https://github.com/acdlite) in
[facebook#26512](facebook#26512))
* Remove unnecessary CIRCLE_CI_API_TOKEN checks
([mondaychen](https://github.com/mondaychen) in
[facebook#26499](facebook#26499))
* browser extension: improve script injection logic
([mondaychen](https://github.com/mondaychen) in
[facebook#26492](facebook#26492))
* [flow] make Flow suppressions explicit on the error
([kassens](https://github.com/kassens) in
[facebook#26487](facebook#26487))
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
@acdlite
DevTools: Inline references to fiber flags (#26542)
04d482a 
We shouldn't be referencing internal fields like fiber's `flag` directly
of DevTools. It's an implementation detail. However, over the years a
few of these have snuck in. Because of how DevTools is currently
shipped, where it's expected to be backwards compatible with older
versions of React, this prevents us from refactoring those fields inside
the reconciler.

The plan we have to address this is to fix how DevTools is shipped:
DevTools will be released in lockstep with each version of React.

Until then, though, I need a temporary solution because it's blocking a
feature I'm working on. So in meantime, I'm going to have to fork the
DevTool's code based on the React version, like we already do with the
fiber TypeOfWork enum.

As a first step, I've inlined all the references to fiber flags into the
specific call sites where they are used. Eventually we'll import these
functions from the reconciler so they stay in sync, rather than
maintaining duplicate copies of the logic.

DiffTrain build for commit 0ba4d7b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mondaychen mondaychen mondaychen approved these changes

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.

None yet
3 participants
@acdlite @mondaychen @facebook-github-bot