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
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
@@ -719,6 +719,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
4,
@@ -1183,6 +1185,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
4,
@@ -1658,6 +1662,8 @@ Object {
1,
13,
11,
0,
1,
1,
1,
4,
@@ -2202,6 +2208,8 @@ Object {
1,
13,
11,
0,
1,
1,
1,
4,
@@ -2295,6 +2303,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
1,
@@ -2943,6 +2953,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
1,
@@ -4214,6 +4226,8 @@ Object {
1,
1,
11,
0,
1,
1,
1,
1,
Original file line number Diff line number Diff line change
@@ -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
@@ -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;
Original file line number Diff line number Diff line change
@@ -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);
29 changes: 29 additions & 0 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
@@ -24,6 +24,7 @@ import {
ElementTypeRoot,
ElementTypeSuspense,
ElementTypeSuspenseList,
StrictMode,
} from 'react-devtools-shared/src/types';
import {
deletePathInObject,
@@ -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';
@@ -155,6 +157,7 @@ export function getInternalReactConstants(
ReactPriorityLevels: ReactPriorityLevelsType,
ReactTypeOfSideEffect: ReactTypeOfSideEffectType,
ReactTypeOfWork: WorkTagMap,
StrictModeBits: number,
|} {
const ReactTypeOfSideEffect: ReactTypeOfSideEffectType = {
DidCapture: 0b10000000,
@@ -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);

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

@@ -534,6 +550,7 @@ export function attach(
ReactPriorityLevels,
ReactTypeOfWork,
ReactTypeOfSideEffect,
StrictModeBits,
} = getInternalReactConstants(version);
const {
DidCapture,
@@ -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) {
@@ -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) {
6 changes: 6 additions & 0 deletions packages/react-devtools-shared/src/bridge.js
Original file line number Diff line number Diff line change
@@ -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,
},
];
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/constants.js
Original file line number Diff line number Diff line change
@@ -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';

56 changes: 55 additions & 1 deletion packages/react-devtools-shared/src/devtools/store.js
Original file line number Diff line number Diff line change
@@ -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';
@@ -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';
@@ -72,6 +74,7 @@ type Config = {|
export type Capabilities = {|
hasOwnerMetadata: boolean,
supportsProfiling: boolean,
supportsStrictMode: boolean,
|};

/**
@@ -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,
@@ -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++;

@@ -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,
@@ -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,
};
@@ -1050,6 +1081,7 @@ export default class Store extends EventEmitter<{|
haveErrorsOrWarningsChanged = true;
}
}

break;
}
case TREE_OPERATION_REMOVE_ROOT: {
@@ -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.
Loading