Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Fixed invalid DevTools work tags #20362
Merged
+84
−19
Conversation
| "build-for-devtools-dev": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom/index,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_DEV", | ||
| "build-for-devtools-prod": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom/index,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_PROD", | ||
| "build-for-devtools-dev": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_DEV", | ||
| "build-for-devtools-prod": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_PROD", |
bvaughn
Dec 1, 2020
Author
Contributor
react-dom/index wouldn't build test utils, which DevTools tests need.
react-dom/index wouldn't build test utils, which DevTools tests need.
| // | ||
| // TODO Update the gt() check below to be gte() whichever the next version number is. | ||
| // Currently the version in Git is 17.0.2 (but that version has not been/may not end up being released). | ||
| if (gt(version, '17.0.1')) { |
bvaughn
Dec 1, 2020
Author
Contributor
We could probably leave the gt() check as-is, but I think gte() is better because it's more explicit. Also if we next released 17.1.0 next, and then went back and bug fix released 17.0.2 from a fork of 17.0.1 (unlikely but possible) this gt() check would fail.
We could probably leave the gt() check as-is, but I think gte() is better because it's more explicit. Also if we next released 17.1.0 next, and then went back and bug fix released 17.0.2 from a fork of 17.0.1 (unlikely but possible) this gt() check would fail.
Work tags changed recently (PR #13902) but we didn't bump React versions. This meant that DevTools has valid work tags only for master (and FB www sync) but invalid work tags for the latest open source releases. To fix this, I incremneted React's version in Git (without an actual release) and added a new fork to the work tags detection branch. This commit also adds tags for the experimental Scope and Fundamental APIs to DevTools so component names will at least display correctly. Technically these new APIs were first introduced to experimental builds ~16.9 but I didn't add a new branch to the work tags fork because I don't they're used commonly. I've just added them to the 17+ branches.
| @@ -354,11 +403,21 @@ export function getInternalReactConstants( | |||
| case HostText: | |||
| case Fragment: | |||
| return null; | |||
| case FundamentalComponent: | |||
| return 'Fundamental'; | |||
bvaughn
Dec 1, 2020
Author
Contributor
There's more work to do to fully support APIs like fundamental and scope, but as long as they exist and are being used within Facebook– at least now they won't show up as "Unknown" in DevTools.
PS I assume we still intend to use both APIs? I haven't been following along with either TBH.
There's more work to do to fully support APIs like fundamental and scope, but as long as they exist and are being used within Facebook– at least now they won't show up as "Unknown" in DevTools.
PS I assume we still intend to use both APIs? I haven't been following along with either TBH.
gaearon
Dec 1, 2020
Member
I don't think we ever used Fundamental. It's dead code.
I don't think we ever used Fundamental. It's dead code.
bvaughn
Dec 1, 2020
Author
Contributor
Excellent. Thanks.
Excellent. Thanks.
Merged
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 127381d:
|
Details of bundled changes.Comparing: 2cf8f7f...127381d react-dom
react-art
react-native-renderer
react-test-renderer
react
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: 2cf8f7f...127381d react-dom
react-art
react-native-renderer
react-test-renderer
react
ReactDOM: size: 0.0%, gzip: -0.0% React: size: 0.0%, gzip: -0.0% Size changes (stable) |
| case FundamentalComponent: | ||
| return 'Fundamental'; | ||
| case LazyComponent: | ||
| return 'Lazy'; |
gaearon
Dec 1, 2020
Member
Should this not show the inner type? Or does it mean it hasn't yet resolved?
Should this not show the inner type? Or does it mean it hasn't yet resolved?
bvaughn
Dec 1, 2020
•
Author
Contributor
I think it means it hasn't resolved yet. Once it resolves, it will be replaced with the inner component:
react/packages/react-reconciler/src/ReactFiberBeginWork.new.js
Lines 1119 to 1121
in
3f73dce
So this label likely wouldn't be user-visible, but could show up in the console logs if we enabled the DevTools DEBUG flag for example.
I actually noticed this while tracking down what I think may be a bug with Lazy (if you unmount a Lazy component before it resolves, React ends up telling DevTools to unmount something that was never mounted and DevTools throws). I'll push a fix for that in a stacked PR shortly. Writing some more tests for it now.
I think it means it hasn't resolved yet. Once it resolves, it will be replaced with the inner component:
react/packages/react-reconciler/src/ReactFiberBeginWork.new.js
Lines 1119 to 1121 in 3f73dce
So this label likely wouldn't be user-visible, but could show up in the console logs if we enabled the DevTools DEBUG flag for example.
I actually noticed this while tracking down what I think may be a bug with Lazy (if you unmount a Lazy component before it resolves, React ends up telling DevTools to unmount something that was never mounted and DevTools throws). I'll push a fix for that in a stacked PR shortly. Writing some more tests for it now.
|
You can remove Fundamental there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Work tags changed recently (PR #13902) but we didn't bump React versions. This meant that DevTools has valid work tags only for master (and FB www sync) but invalid work tags for the latest open source releases. To fix this, I incremented React's version in Git (without an actual release) and added a new fork to the work tags detection branch.
This commit also add explicit tags for a few missing types (Fundamental, Lazy, LegacyHidden, Offscreen, and Scope) so meaningful names will be shown rather than "Unknown". (Technically Fundamental and Scope APIs were first introduced to experimental builds ~16.9 but I didn't add a new branch to the work tags fork because I don't they're used commonly. I've just added them to the 17+ branches.)