Skip to content

Commit

Permalink
Advocate for StrictMode usage within Components tree (facebook#22886)
Browse files Browse the repository at this point in the history 
Adds the concept of subtree modes to DevTools to bridge protocol as follows:
1. Add-root messages get two new attributes: one specifying whether the root is running in strict mode and another specifying whether the root (really the root's renderer) supports the concept of strict mode.
2. A new backend message type (TREE_OPERATION_SET_SUBTREE_MODE). This type specifies a subtree root (id) and a mode (bitmask). For now, the only mode this message deals with is strict mode.

The DevTools frontend has been updated as well to highlight non-StrictMode compliant components.

The changes to the bridge protocol require incrementing the bridge protocol version number, which will also require updating the version of react-devtools-core backend that is shipped with React Native.
  • Loading branch information
@zhengjitf
Brian Vaughn authored and zhengjitf committed Apr 15, 2022
1 parent 75a6b5c commit 019cadd
Show file tree
Hide file tree
Showing 20 changed files with 370 additions and 23 deletions.
14 changes: 14 additions & 0 deletions packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
4,
Expand Down Expand Up @@ -1183,6 +1185,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
4,
Expand Down Expand Up @@ -1658,6 +1662,8 @@ Object {
1,
13,
11,
0,
1,
1,
1,
4,
Expand Down Expand Up @@ -2202,6 +2208,8 @@ Object {
1,
13,
11,
0,
1,
1,
1,
4,
Expand Down Expand Up @@ -2295,6 +2303,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
1,
Expand Down Expand Up @@ -2943,6 +2953,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
1,
Expand Down Expand Up @@ -4214,6 +4226,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
1,
Expand Down
11 changes: 11 additions & 0 deletions packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,4 +509,15 @@ describe('Store (legacy)', () => {
expect(store).toMatchSnapshot('5: collapse root');
});
});

describe('StrictMode compliance', () => {
it('should mark all elements as strict mode compliant', () => {
const App = () => null;

const container = document.createElement('div');
act(() => ReactDOM.render(<App />, container));

expect(store.getElementAtIndex(0).isStrictModeNonCompliant).toBe(false);
});
});
});
48 changes: 48 additions & 0 deletions packages/react-devtools-shared/src/__tests__/store-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,54 @@ describe('Store', () => {
`);
});

describe('StrictMode compliance', () => {
it('should mark strict root elements as strict', () => {
const App = () => <Component />;
const Component = () => null;

const container = document.createElement('div');
const root = ReactDOM.createRoot(container, {unstable_strictMode: true});
act(() => {
root.render(<App />);
});

expect(store.getElementAtIndex(0).isStrictModeNonCompliant).toBe(false);
expect(store.getElementAtIndex(1).isStrictModeNonCompliant).toBe(false);
});

it('should mark non strict root elements as not strict', () => {
const App = () => <Component />;
const Component = () => null;

const container = document.createElement('div');
const root = ReactDOM.createRoot(container);
act(() => {
root.render(<App />);
});

expect(store.getElementAtIndex(0).isStrictModeNonCompliant).toBe(true);
expect(store.getElementAtIndex(1).isStrictModeNonCompliant).toBe(true);
});

it('should mark StrictMode subtree elements as strict', () => {
const App = () => (
<React.StrictMode>
<Component />
</React.StrictMode>
);
const Component = () => null;

const container = document.createElement('div');
const root = ReactDOM.createRoot(container);
act(() => {
root.render(<App />);
});

expect(store.getElementAtIndex(0).isStrictModeNonCompliant).toBe(true);
expect(store.getElementAtIndex(1).isStrictModeNonCompliant).toBe(false);
});
});

describe('collapseNodesByDefault:false', () => {
beforeEach(() => {
store.collapseNodesByDefault = false;
Expand Down
4 changes: 3 additions & 1 deletion packages/react-devtools-shared/src/backend/legacy/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,9 @@ export function attach(
pushOperation(TREE_OPERATION_ADD);
pushOperation(id);
pushOperation(ElementTypeRoot);
pushOperation(0); // isProfilingSupported?
pushOperation(0); // StrictMode compliant?
pushOperation(0); // Profiling supported?
pushOperation(0); // StrictMode supported?
pushOperation(hasOwnerMetadata ? 1 : 0);
} else {
const type = getElementType(internalInstance);
Expand Down
29 changes: 29 additions & 0 deletions packages/react-devtools-shared/src/backend/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
ElementTypeRoot,
ElementTypeSuspense,
ElementTypeSuspenseList,
StrictMode,
} from 'react-devtools-shared/src/types';
import {
deletePathInObject,
Expand Down Expand Up @@ -52,6 +53,7 @@ import {
TREE_OPERATION_REMOVE,
TREE_OPERATION_REMOVE_ROOT,
TREE_OPERATION_REORDER_CHILDREN,
TREE_OPERATION_SET_SUBTREE_MODE,
TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS,
TREE_OPERATION_UPDATE_TREE_BASE_DURATION,
} from '../constants';
Expand Down Expand Up @@ -155,6 +157,7 @@ export function getInternalReactConstants(
ReactPriorityLevels: ReactPriorityLevelsType,
ReactTypeOfSideEffect: ReactTypeOfSideEffectType,
ReactTypeOfWork: WorkTagMap,
StrictModeBits: number,
|} {
const ReactTypeOfSideEffect: ReactTypeOfSideEffectType = {
DidCapture: 0b10000000,
Expand Down Expand Up @@ -192,6 +195,18 @@ export function getInternalReactConstants(
};
}

let StrictModeBits = 0;
if (gte(version, '18.0.0-alpha')) {
// 18+
StrictModeBits = 0b011000;
} else if (gte(version, '16.9.0')) {
// 16.9 - 17
StrictModeBits = 0b1;
} else if (gte(version, '16.3.0')) {
// 16.3 - 16.8
StrictModeBits = 0b10;
}

let ReactTypeOfWork: WorkTagMap = ((null: any): WorkTagMap);

// **********************************************************
Expand Down Expand Up @@ -513,6 +528,7 @@ export function getInternalReactConstants(
ReactPriorityLevels,
ReactTypeOfWork,
ReactTypeOfSideEffect,
StrictModeBits,
};
}

Expand All @@ -534,6 +550,7 @@ export function attach(
ReactPriorityLevels,
ReactTypeOfWork,
ReactTypeOfSideEffect,
StrictModeBits,
} = getInternalReactConstants(version);
const {
DidCapture,
Expand Down Expand Up @@ -1876,7 +1893,9 @@ export function attach(
pushOperation(TREE_OPERATION_ADD);
pushOperation(id);
pushOperation(ElementTypeRoot);
pushOperation((fiber.mode & StrictModeBits) !== 0 ? 1 : 0);
pushOperation(isProfilingSupported ? 1 : 0);
pushOperation(StrictModeBits !== 0 ? 1 : 0);
pushOperation(hasOwnerMetadata ? 1 : 0);

if (isProfiling) {
Expand Down Expand Up @@ -1913,6 +1932,16 @@ export function attach(
pushOperation(ownerID);
pushOperation(displayNameStringID);
pushOperation(keyStringID);

// If this subtree has a new mode, let the frontend know.
if (
(fiber.mode & StrictModeBits) !== 0 &&
(((parentFiber: any): Fiber).mode & StrictModeBits) === 0
) {
pushOperation(TREE_OPERATION_SET_SUBTREE_MODE);
pushOperation(id);
pushOperation(StrictMode);
}
}

if (isProfilingSupported) {
Expand Down
6 changes: 6 additions & 0 deletions packages/react-devtools-shared/src/bridge.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ export const BRIDGE_PROTOCOL: Array<BridgeProtocol> = [
{
version: 1,
minNpmVersion: '4.13.0',
maxNpmVersion: '4.21.0',
},
// Version 2 adds a StrictMode-enabled and supports-StrictMode bits to add-root operation.
{
version: 2,
minNpmVersion: '4.22.0',
maxNpmVersion: null,
},
];
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/constants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const TREE_OPERATION_REORDER_CHILDREN = 3;
export const TREE_OPERATION_UPDATE_TREE_BASE_DURATION = 4;
export const TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS = 5;
export const TREE_OPERATION_REMOVE_ROOT = 6;
export const TREE_OPERATION_SET_SUBTREE_MODE = 7;

export const LOCAL_STORAGE_DEFAULT_TAB_KEY = 'React::DevTools::defaultTab';

Expand Down
56 changes: 55 additions & 1 deletion packages/react-devtools-shared/src/devtools/store.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
TREE_OPERATION_REMOVE,
TREE_OPERATION_REMOVE_ROOT,
TREE_OPERATION_REORDER_CHILDREN,
TREE_OPERATION_SET_SUBTREE_MODE,
TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS,
TREE_OPERATION_UPDATE_TREE_BASE_DURATION,
} from '../constants';
Expand All @@ -33,6 +34,7 @@ import {
BRIDGE_PROTOCOL,
currentBridgeProtocol,
} from 'react-devtools-shared/src/bridge';
import {StrictMode} from 'react-devtools-shared/src/types';

import type {Element} from './views/Components/types';
import type {ComponentFilter, ElementType} from '../types';
Expand Down Expand Up @@ -72,6 +74,7 @@ type Config = {|
export type Capabilities = {|
hasOwnerMetadata: boolean,
supportsProfiling: boolean,
supportsStrictMode: boolean,
|};

/**
Expand Down Expand Up @@ -812,6 +815,20 @@ export default class Store extends EventEmitter<{|
}
};

_recursivelyUpdateSubtree(
id: number,
callback: (element: Element) => void,
): void {
const element = this._idToElement.get(id);
if (element) {
callback(element);

element.children.forEach(child =>
this._recursivelyUpdateSubtree(child, callback),
);
}
}

onBridgeNativeStyleEditorSupported = ({
isSupported,
validAttributes,
Expand Down Expand Up @@ -883,9 +900,15 @@ export default class Store extends EventEmitter<{|
debug('Add', `new root node ${id}`);
}

const isStrictModeCompliant = operations[i] > 0;
i++;

const supportsProfiling = operations[i] > 0;
i++;

const supportsStrictMode = operations[i] > 0;
i++;

const hasOwnerMetadata = operations[i] > 0;
i++;

Expand All @@ -894,15 +917,22 @@ export default class Store extends EventEmitter<{|
this._rootIDToCapabilities.set(id, {
hasOwnerMetadata,
supportsProfiling,
supportsStrictMode,
});

// Not all roots support StrictMode;
// don't flag a root as non-compliant unless it also supports StrictMode.
const isStrictModeNonCompliant =
!isStrictModeCompliant && supportsStrictMode;

this._idToElement.set(id, {
children: [],
depth: -1,
displayName: null,
hocDisplayNames: null,
id,
isCollapsed: false, // Never collapse roots; it would hide the entire tree.
isStrictModeNonCompliant,
key: null,
ownerID: 0,
parentID: 0,
Expand Down Expand Up @@ -958,9 +988,10 @@ export default class Store extends EventEmitter<{|
hocDisplayNames,
id,
isCollapsed: this._collapseNodesByDefault,
isStrictModeNonCompliant: parentElement.isStrictModeNonCompliant,
key,
ownerID,
parentID: parentElement.id,
parentID,
type,
weight: 1,
};
Expand Down Expand Up @@ -1050,6 +1081,7 @@ export default class Store extends EventEmitter<{|
haveErrorsOrWarningsChanged = true;
}
}

break;
}
case TREE_OPERATION_REMOVE_ROOT: {
Expand Down Expand Up @@ -1124,6 +1156,28 @@ export default class Store extends EventEmitter<{|
}
break;
}
case TREE_OPERATION_SET_SUBTREE_MODE: {
const id = operations[i + 1];
const mode = operations[i + 2];

i += 3;

// If elements have already been mounted in this subtree, update them.
// (In practice, this likely only applies to the root element.)
if (mode === StrictMode) {
this._recursivelyUpdateSubtree(id, element => {
element.isStrictModeNonCompliant = false;
});
}

if (__DEBUG__) {
debug(
'Subtree mode',
`Subtree with root ${id} set to mode ${mode}`,
);
}
break;
}
case TREE_OPERATION_UPDATE_TREE_BASE_DURATION:
// Base duration updates are only sent while profiling is in progress.
// We can ignore them at this point.
Expand Down
Loading

0 comments on commit 019cadd

Please sign in to comment.