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

Fixed invalid DevTools work tags #20362

Merged
merged 2 commits into from
Dec 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@
},
"scripts": {
"build": "node ./scripts/rollup/build.js",
"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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-dom/index wouldn't build test utils, which DevTools tests need.

"linc": "node ./scripts/tasks/linc.js",
"lint": "node ./scripts/tasks/eslint.js",
"lint-build": "node ./scripts/rollup/validate/index.js",
Expand Down
4 changes: 2 additions & 2 deletions packages/react-art/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "react-art",
"description": "React ART is a JavaScript library for drawing vector graphics using React. It provides declarative and reactive bindings to the ART library. Using the same declarative API you can render the output to either Canvas, SVG or VML (IE8).",
"version": "17.0.1",
"version": "17.0.2",
"main": "index.js",
"repository": {
"type": "git",
Expand Down Expand Up @@ -29,7 +29,7 @@
"scheduler": "^0.20.1"
},
"peerDependencies": {
"react": "17.0.1"
"react": "17.0.2"
},
"files": [
"LICENSE",
Expand Down
71 changes: 65 additions & 6 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import {gte} from 'semver';
import {gt, gte} from 'semver';
import {
ComponentFilterDisplayName,
ComponentFilterElementType,
Expand Down Expand Up @@ -166,8 +166,10 @@ export function getInternalReactConstants(
// **********************************************************
// The section below is copied from files in React repo.
// Keep it in sync, and add version guards if it changes.
if (gte(version, '17.0.0-alpha')) {
// TODO (Offscreen) Update the version number above to reflect the first Offscreen alpha/beta release.
//
// 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')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

ReactTypeOfWork = {
ClassComponent: 1,
ContextConsumer: 9,
Expand All @@ -185,10 +187,41 @@ export function getInternalReactConstants(
IncompleteClassComponent: 17,
IndeterminateComponent: 2,
LazyComponent: 16,
LegacyHiddenComponent: 23,
MemoComponent: 14,
Mode: 8,
OffscreenComponent: 22, // Experimental
Profiler: 12,
ScopeComponent: 21, // Experimental
SimpleMemoComponent: 15,
SuspenseComponent: 13,
SuspenseListComponent: 19, // Experimental
YieldComponent: -1, // Removed
};
} else if (gte(version, '17.0.0-alpha')) {
ReactTypeOfWork = {
ClassComponent: 1,
ContextConsumer: 9,
ContextProvider: 10,
CoroutineComponent: -1, // Removed
CoroutineHandlerPhase: -1, // Removed
DehydratedSuspenseComponent: 18, // Behind a flag
ForwardRef: 11,
Fragment: 7,
FunctionComponent: 0,
HostComponent: 5,
HostPortal: 4,
HostRoot: 3,
HostText: 6,
IncompleteClassComponent: 17,
IndeterminateComponent: 2,
LazyComponent: 16,
LegacyHiddenComponent: 24,
MemoComponent: 14,
Mode: 8,
OffscreenComponent: 23, // Experimental
Profiler: 12,
ScopeComponent: 21, // Experimental
SimpleMemoComponent: 15,
SuspenseComponent: 13,
SuspenseListComponent: 19, // Experimental
Expand All @@ -212,10 +245,12 @@ export function getInternalReactConstants(
IncompleteClassComponent: 17,
IndeterminateComponent: 2,
LazyComponent: 16,
LegacyHiddenComponent: -1,
MemoComponent: 14,
Mode: 8,
OffscreenComponent: -1, // Experimental
Profiler: 12,
ScopeComponent: -1, // Experimental
SimpleMemoComponent: 15,
SuspenseComponent: 13,
SuspenseListComponent: 19, // Experimental
Expand All @@ -239,10 +274,12 @@ export function getInternalReactConstants(
IncompleteClassComponent: -1, // Doesn't exist yet
IndeterminateComponent: 4,
LazyComponent: -1, // Doesn't exist yet
LegacyHiddenComponent: -1,
MemoComponent: -1, // Doesn't exist yet
Mode: 10,
OffscreenComponent: -1, // Experimental
Profiler: 15,
ScopeComponent: -1, // Experimental
SimpleMemoComponent: -1, // Doesn't exist yet
SuspenseComponent: 16,
SuspenseListComponent: -1, // Doesn't exist yet
Expand All @@ -266,10 +303,12 @@ export function getInternalReactConstants(
IncompleteClassComponent: -1, // Doesn't exist yet
IndeterminateComponent: 0,
LazyComponent: -1, // Doesn't exist yet
LegacyHiddenComponent: -1,
MemoComponent: -1, // Doesn't exist yet
Mode: 11,
OffscreenComponent: -1, // Experimental
Profiler: 15,
ScopeComponent: -1, // Experimental
SimpleMemoComponent: -1, // Doesn't exist yet
SuspenseComponent: 16,
SuspenseListComponent: -1, // Doesn't exist yet
Expand Down Expand Up @@ -301,7 +340,11 @@ export function getInternalReactConstants(
HostPortal,
HostText,
Fragment,
LazyComponent,
LegacyHiddenComponent,
MemoComponent,
OffscreenComponent,
ScopeComponent,
SimpleMemoComponent,
SuspenseComponent,
SuspenseListComponent,
Expand Down Expand Up @@ -354,11 +397,22 @@ export function getInternalReactConstants(
case HostText:
case Fragment:
return null;
case LazyComponent:
// This display name will not be user visible.
// Once a Lazy component loads its inner component, React replaces the tag and type.
// This display name will only show up in console logs when DevTools DEBUG mode is on.
return 'Lazy';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not show the inner type? Or does it mean it hasn't yet resolved?

Copy link
Contributor Author

@bvaughn bvaughn Dec 1, 2020

Choose a reason for hiding this comment

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

I think it means it hasn't resolved yet. Once it resolves, it will be replaced with the inner component:

// Store the unwrapped component in the type.
workInProgress.type = Component;
const resolvedTag = (workInProgress.tag = resolveLazyComponentTag(Component));

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.

case MemoComponent:
case SimpleMemoComponent:
return getDisplayName(resolvedType, 'Anonymous');
case SuspenseComponent:
return 'Suspense';
case LegacyHiddenComponent:
return 'LegacyHidden';
case OffscreenComponent:
return 'Offscreen';
case ScopeComponent:
return 'Scope';
case SuspenseListComponent:
return 'SuspenseList';
default:
Expand Down Expand Up @@ -493,10 +547,14 @@ export function attach(

const debug = (name: string, fiber: Fiber, parentFiber: ?Fiber): void => {
if (__DEBUG__) {
const displayName = getDisplayNameForFiber(fiber) || 'null';
const displayName =
fiber.tag + ':' + (getDisplayNameForFiber(fiber) || 'null');
const id = getFiberID(fiber);
const parentDisplayName =
(parentFiber != null && getDisplayNameForFiber(parentFiber)) || 'null';
const parentDisplayName = parentFiber
? parentFiber.tag +
':' +
(getDisplayNameForFiber(parentFiber) || 'null')
: '';
const parentID = parentFiber ? getFiberID(parentFiber) : '';
// NOTE: calling getFiberID or getPrimaryFiber is unsafe here
// because it will put them in the map. For now, we'll omit them.
Expand Down Expand Up @@ -1207,6 +1265,7 @@ export function attach(
return;
}
const id = getFiberID(primaryFiber);

if (isRoot) {
// Roots must be removed only after all children (pending and simulated) have been removed.
// So we track it separately.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-devtools-shared/src/backend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,12 @@ export type WorkTagMap = {|
IncompleteClassComponent: WorkTag,
IndeterminateComponent: WorkTag,
LazyComponent: WorkTag,
LegacyHiddenComponent: WorkTag,
MemoComponent: WorkTag,
Mode: WorkTag,
OffscreenComponent: WorkTag,
Profiler: WorkTag,
ScopeComponent: WorkTag,
SimpleMemoComponent: WorkTag,
SuspenseComponent: WorkTag,
SuspenseListComponent: WorkTag,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-dom",
"version": "17.0.1",
"version": "17.0.2",
"description": "React package for working with the DOM.",
"main": "index.js",
"repository": {
Expand All @@ -22,7 +22,7 @@
"scheduler": "^0.20.1"
},
"peerDependencies": {
"react": "17.0.1"
"react": "17.0.2"
},
"files": [
"LICENSE",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-is/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-is",
"version": "17.0.1",
"version": "17.0.2",
"description": "Brand checking of React Elements.",
"main": "index.js",
"repository": {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"node": ">=0.10.0"
},
"peerDependencies": {
"react": "^17.0.1"
"react": "^17.0.2"
},
"dependencies": {
"loose-envify": "^1.1.0",
Expand Down
6 changes: 3 additions & 3 deletions packages/react-test-renderer/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-test-renderer",
"version": "17.0.1",
"version": "17.0.2",
"description": "React package for snapshot testing.",
"main": "index.js",
"repository": {
Expand All @@ -20,12 +20,12 @@
"homepage": "https://reactjs.org/",
"dependencies": {
"object-assign": "^4.1.1",
"react-is": "^17.0.1",
"react-is": "^17.0.2",
"react-shallow-renderer": "^16.13.1",
"scheduler": "^0.20.1"
},
"peerDependencies": {
"react": "17.0.1"
"react": "17.0.2"
},
"files": [
"LICENSE",
Expand Down
2 changes: 1 addition & 1 deletion packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"keywords": [
"react"
],
"version": "17.0.1",
"version": "17.0.2",
"homepage": "https://reactjs.org/",
"bugs": "https://github.com/facebook/react/issues",
"license": "MIT",
Expand Down
6 changes: 5 additions & 1 deletion packages/shared/ReactVersion.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@
*/

// TODO: this is special because it gets imported during build.
export default '17.0.1';
//
// TODO: 17.0.2 has not been released to NPM;
// It exists as a placeholder so that DevTools can support work tag changes between releases.
// When we next publish a release (either 17.0.2 or 17.1.0), update the matching TODO in backend/renderer.js
export default '17.0.2';