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

Advocate for StrictMode usage within Components tree #22886

Merged
merged 3 commits into from
Dec 10, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
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
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
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
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
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;
}

bvaughn marked this conversation as resolved.
Show resolved Hide resolved
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
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
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
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