diff --git a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js index 257eb8554a736..db0066582e69c 100644 --- a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js +++ b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js @@ -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; }; @@ -167,7 +173,20 @@ describe('ReactInternalTestUtils', () => { ); }); - 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']); }); }); diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js index e8f5376c7bf2e..e7a60ea369560 100644 --- a/packages/react-devtools-shared/src/__tests__/console-test.js +++ b/packages/react-devtools-shared/src/__tests__/console-test.js @@ -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
; + } + + act(() => + root.render( + + + , + ), + ); + 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
; + } + + act(() => + root.render( + + + , + ), + ); + 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; diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index 27e212b33a935..40649abe9ee46 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -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 = [ @@ -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) { diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js b/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js index 778bb91354a33..b28ca67c0bcb4 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js @@ -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{' '} + + Strict Mode +
diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsShared.css b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsShared.css index c803f611908f4..c322bc8d0eaab 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsShared.css +++ b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsShared.css @@ -141,7 +141,7 @@ border-radius: 0.25rem; } -.ReleaseNotesLink { +.ReleaseNotesLink, .StrictModeLink { color: var(--color-button-active); } @@ -153,4 +153,4 @@ list-style: none; padding: 0; margin: 0; -} \ No newline at end of file +} diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 3ab3f1b6826bc..3f40efaf218c5 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -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, }: { @@ -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; @@ -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(); } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index e68b4de70f995..f47f079c8a3bb 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -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'; @@ -3675,6 +3676,7 @@ function doubleInvokeEffectsOnFiber( fiber: Fiber, shouldDoubleInvokePassiveEffects: boolean = true, ) { + setIsStrictModeForDevtools(true); disappearLayoutEffects(fiber); if (shouldDoubleInvokePassiveEffects) { disconnectPassiveEffect(fiber); @@ -3683,6 +3685,7 @@ function doubleInvokeEffectsOnFiber( if (shouldDoubleInvokePassiveEffects) { reconnectPassiveEffects(root, fiber, NoLanes, null, false); } + setIsStrictModeForDevtools(false); } function doubleInvokeEffectsInDEVIfNecessary( diff --git a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js index 7d1a3b77cb543..c9f65a20a0c7b 100644 --- a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js +++ b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js @@ -115,11 +115,7 @@ describe('StrictEffectsMode defaults', () => { , ); - 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"', @@ -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. @@ -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"', @@ -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"', diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index eabf2a7bd59bb..d9d6815b88e5a 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -1343,6 +1343,42 @@ describe('context legacy', () => { // and on the next render they'd get deduplicated and ignored. expect(console.log).toBeCalledWith('foo 1'); }); + + it('does not disable logs for effect double invoke', async () => { + let create = 0; + let cleanup = 0; + function Foo() { + React.useEffect(() => { + create++; + console.log('foo create ' + create); + return () => { + cleanup++; + console.log('foo cleanup ' + cleanup); + }; + }); + return null; + } + + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( + + + , + ); + }); + expect(create).toBe(__DEV__ ? 2 : 1); + expect(cleanup).toBe(__DEV__ ? 1 : 0); + expect(console.log).toBeCalledTimes(__DEV__ ? 3 : 1); + // Note: we should display the first log because otherwise + // there is a risk of suppressing warnings when they happen, + // and on the next render they'd get deduplicated and ignored. + expect(console.log).toBeCalledWith('foo create 1'); + if (__DEV__) { + expect(console.log).toBeCalledWith('foo cleanup 1'); + } + }); } else { it('disable logs for class double render', async () => { let count = 0; @@ -1530,6 +1566,39 @@ describe('context legacy', () => { // and on the next render they'd get deduplicated and ignored. expect(console.log).toBeCalledWith('foo 1'); }); + + it('disable logs for effect double invoke', async () => { + let create = 0; + let cleanup = 0; + function Foo() { + React.useEffect(() => { + create++; + console.log('foo create ' + create); + return () => { + cleanup++; + console.log('foo cleanup ' + cleanup); + }; + }); + return null; + } + + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( + + + , + ); + }); + expect(create).toBe(__DEV__ ? 2 : 1); + expect(cleanup).toBe(__DEV__ ? 1 : 0); + expect(console.log).toBeCalledTimes(1); + // Note: we should display the first log because otherwise + // there is a risk of suppressing warnings when they happen, + // and on the next render they'd get deduplicated and ignored. + expect(console.log).toBeCalledWith('foo create 1'); + }); } }); });