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.

Sign up for GitHub

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

Jump to bottom

Dim console calls on additional Effect invocations due to StrictMode #29007

Merged
merged 5 commits into from
May 22, 2024
Merged

Dim console calls on additional Effect invocations due to StrictMode #29007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2814b30
Rename RDT option
eps1lon May 6, 2024
03efa54
Current behavior of console calls during second invocation of Effects…
eps1lon May 6, 2024
4105752
Dim `console` calls when invoking effects due to Strict Mode
eps1lon May 6, 2024
ed7a2b2
Linkify "Strict Mode"
eps1lon May 20, 2024
8eb27a1
`patchConsoleForInitialRenderInStrictMode`-> `patchConsoleForInitialC…
eps1lon May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ describe('ReactInternalTestUtils', () => {
test('assertLog', async () => {
const Yield = ({id}) => {
Scheduler.log(id);
React.useEffect(() => {
Scheduler.log(`create effect ${id}`);
return () => {
Scheduler.log(`cleanup effect ${id}`);
};
});
return id;
};

Expand All @@ -167,7 +173,20 @@ describe('ReactInternalTestUtils', () => {
</React.StrictMode>
);
});
assertLog(['A', 'B', 'C']);
assertLog([
'A',
'B',
'C',
'create effect A',
'create effect B',
'create effect C',
]);

await act(() => {
root.render(null);
});

assertLog(['cleanup effect A', 'cleanup effect B', 'cleanup effect C']);
});
});

Expand Down
162 changes: 162 additions & 0 deletions packages/react-devtools-shared/src/__tests__/console-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,168 @@ describe('console', () => {
expect(mockGroupCollapsed.mock.calls[0][0]).toBe('groupCollapsed');
});

it('should double log from Effects if hideConsoleLogsInStrictMode is disabled in Strict mode', () => {
global.__REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ = false;
global.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__ = false;

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

function App() {
React.useEffect(() => {
fakeConsole.log('log effect create');
fakeConsole.warn('warn effect create');
fakeConsole.error('error effect create');
fakeConsole.info('info effect create');
fakeConsole.group('group effect create');
fakeConsole.groupCollapsed('groupCollapsed effect create');

return () => {
fakeConsole.log('log effect cleanup');
fakeConsole.warn('warn effect cleanup');
fakeConsole.error('error effect cleanup');
fakeConsole.info('info effect cleanup');
fakeConsole.group('group effect cleanup');
fakeConsole.groupCollapsed('groupCollapsed effect cleanup');
};
});

return <div />;
}

act(() =>
root.render(
<React.StrictMode>
<App />
</React.StrictMode>,
),
);
expect(mockLog.mock.calls).toEqual([
['log effect create'],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'log effect cleanup',
],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'log effect create',
],
]);
expect(mockWarn.mock.calls).toEqual([
['warn effect create'],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_WARNING_COLOR}`,
'warn effect cleanup',
],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_WARNING_COLOR}`,
'warn effect create',
],
]);
expect(mockError.mock.calls).toEqual([
['error effect create'],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_ERROR_COLOR}`,
'error effect cleanup',
],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_ERROR_COLOR}`,
'error effect create',
],
]);
expect(mockInfo.mock.calls).toEqual([
['info effect create'],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'info effect cleanup',
],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'info effect create',
],
]);
expect(mockGroup.mock.calls).toEqual([
['group effect create'],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'group effect cleanup',
],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'group effect create',
],
]);
expect(mockGroupCollapsed.mock.calls).toEqual([
['groupCollapsed effect create'],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'groupCollapsed effect cleanup',
],
[
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'groupCollapsed effect create',
],
]);
});

it('should not double log from Effects if hideConsoleLogsInStrictMode is enabled in Strict mode', () => {
global.__REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ = false;
global.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__ = true;

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

function App() {
React.useEffect(() => {
fakeConsole.log('log effect create');
fakeConsole.warn('warn effect create');
fakeConsole.error('error effect create');
fakeConsole.info('info effect create');
fakeConsole.group('group effect create');
fakeConsole.groupCollapsed('groupCollapsed effect create');

return () => {
fakeConsole.log('log effect cleanup');
fakeConsole.warn('warn effect cleanup');
fakeConsole.error('error effect cleanup');
fakeConsole.info('info effect cleanup');
fakeConsole.group('group effect cleanup');
fakeConsole.groupCollapsed('groupCollapsed effect cleanup');
};
});

return <div />;
}

act(() =>
root.render(
<React.StrictMode>
<App />
</React.StrictMode>,
),
);
expect(mockLog.mock.calls).toEqual([['log effect create']]);
expect(mockWarn.mock.calls).toEqual([['warn effect create']]);
expect(mockError.mock.calls).toEqual([['error effect create']]);
expect(mockInfo.mock.calls).toEqual([['info effect create']]);
expect(mockGroup.mock.calls).toEqual([['group effect create']]);
expect(mockGroupCollapsed.mock.calls).toEqual([
['groupCollapsed effect create'],
]);
});

it('should double log from useMemo if hideConsoleLogsInStrictMode is disabled in Strict mode', () => {
global.__REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ = false;
global.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__ = false;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/backend/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ export function unpatch(): void {

let unpatchForStrictModeFn: null | (() => void) = null;

// NOTE: KEEP IN SYNC with src/hook.js:patchConsoleForInitialRenderInStrictMode
// NOTE: KEEP IN SYNC with src/hook.js:patchConsoleForInitialCommitInStrictMode
export function patchForStrictMode() {
if (consoleManagedByDevToolsDuringStrictMode) {
const overrideConsoleMethods = [
Expand Down Expand Up @@ -359,7 +359,7 @@ export function patchForStrictMode() {
}
}

// NOTE: KEEP IN SYNC with src/hook.js:unpatchConsoleForInitialRenderInStrictMode
// NOTE: KEEP IN SYNC with src/hook.js:unpatchConsoleForInitialCommitInStrictMode
export function unpatchForStrictMode(): void {
if (consoleManagedByDevToolsDuringStrictMode) {
if (unpatchForStrictModeFn !== null) {
Expand Down
9 changes: 8 additions & 1 deletion packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,14 @@ export default function DebuggingSettings(_: {}): React.Node {
setHideConsoleLogsInStrictMode(currentTarget.checked)
}
/>{' '}
Hide logs during second render in Strict Mode
Hide logs during additional invocations in{' '}
<a
className={styles.StrictModeLink}
target="_blank"
rel="noopener noreferrer"
href="https://react.dev/reference/react/StrictMode">
Strict Mode
</a>
</label>
</div>
</div>
Expand Down
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/devtools/views/Settings/SettingsShared.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@
border-radius: 0.25rem;
}

.ReleaseNotesLink {
.ReleaseNotesLink, .StrictModeLink {
color: var(--color-button-active);
}

Expand All @@ -153,4 +153,4 @@
list-style: none;
padding: 0;
margin: 0;
}
}
10 changes: 5 additions & 5 deletions packages/react-devtools-shared/src/hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ export function installHook(target: any): DevToolsHook | null {
// React and DevTools are connecting and the renderer interface isn't avaiable
// and we want to be able to have consistent logging behavior for double logs
// during the initial renderer.
function patchConsoleForInitialRenderInStrictMode({
function patchConsoleForInitialCommitInStrictMode({
hideConsoleLogsInStrictMode,
browserTheme,
}: {
Expand Down Expand Up @@ -311,7 +311,7 @@ export function installHook(target: any): DevToolsHook | null {
}

// NOTE: KEEP IN SYNC with src/backend/console.js:unpatchForStrictMode
function unpatchConsoleForInitialRenderInStrictMode() {
function unpatchConsoleForInitialCommitInStrictMode() {
if (unpatchFn !== null) {
unpatchFn();
unpatchFn = null;
Expand Down Expand Up @@ -451,19 +451,19 @@ export function installHook(target: any): DevToolsHook | null {
rendererInterface.unpatchConsoleForStrictMode();
}
} else {
// This should only happen during initial render in the extension before DevTools
// This should only happen during initial commit in the extension before DevTools
// finishes its handshake with the injected renderer
if (isStrictMode) {
const hideConsoleLogsInStrictMode =
window.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__ === true;
const browserTheme = window.__REACT_DEVTOOLS_BROWSER_THEME__;

patchConsoleForInitialRenderInStrictMode({
patchConsoleForInitialCommitInStrictMode({
hideConsoleLogsInStrictMode,
browserTheme,
});
} else {
unpatchConsoleForInitialRenderInStrictMode();
unpatchConsoleForInitialCommitInStrictMode();
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ import {
markRenderStopped,
onCommitRoot as onCommitRootDevTools,
onPostCommitRoot as onPostCommitRootDevTools,
setIsStrictModeForDevtools,
} from './ReactFiberDevToolsHook';
import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors';
import {releaseCache} from './ReactFiberCacheComponent';
Expand Down Expand Up @@ -3675,6 +3676,7 @@ function doubleInvokeEffectsOnFiber(
fiber: Fiber,
shouldDoubleInvokePassiveEffects: boolean = true,
) {
setIsStrictModeForDevtools(true);
disappearLayoutEffects(fiber);
if (shouldDoubleInvokePassiveEffects) {
disconnectPassiveEffect(fiber);
Expand All @@ -3683,6 +3685,7 @@ function doubleInvokeEffectsOnFiber(
if (shouldDoubleInvokePassiveEffects) {
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
}
setIsStrictModeForDevtools(false);
}

function doubleInvokeEffectsInDEVIfNecessary(
Expand Down
20 changes: 1 addition & 19 deletions packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,7 @@ describe('StrictEffectsMode defaults', () => {
</React.StrictMode>,
);

await waitForPaint([
'useLayoutEffect mount "one"',
'useLayoutEffect unmount "one"',
'useLayoutEffect mount "one"',
]);
await waitForPaint(['useLayoutEffect mount "one"']);
expect(log).toEqual([
'useLayoutEffect mount "one"',
'useLayoutEffect unmount "one"',
Expand All @@ -142,10 +138,6 @@ describe('StrictEffectsMode defaults', () => {
'useLayoutEffect unmount "one"',
'useLayoutEffect mount "one"',
'useLayoutEffect mount "two"',

// Since "two" is new, it should be double-invoked.
'useLayoutEffect unmount "two"',
'useLayoutEffect mount "two"',
]);
expect(log).toEqual([
// Cleanup and re-run "one" (and "two") since there is no dependencies array.
Expand Down Expand Up @@ -196,10 +188,6 @@ describe('StrictEffectsMode defaults', () => {
await waitForAll([
'useLayoutEffect mount "one"',
'useEffect mount "one"',
'useLayoutEffect unmount "one"',
'useEffect unmount "one"',
'useLayoutEffect mount "one"',
'useEffect mount "one"',
]);
expect(log).toEqual([
'useLayoutEffect mount "one"',
Expand Down Expand Up @@ -237,12 +225,6 @@ describe('StrictEffectsMode defaults', () => {
'useEffect unmount "one"',
'useEffect mount "one"',
'useEffect mount "two"',

// Since "two" is new, it should be double-invoked.
'useLayoutEffect unmount "two"',
'useEffect unmount "two"',
'useLayoutEffect mount "two"',
'useEffect mount "two"',
]);
expect(log).toEqual([
'useEffect unmount "one"',
Expand Down