From 8b272df207082e89207b5a006d657485b8cc054c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 14 Oct 2020 12:56:15 -0400 Subject: [PATCH] DevTools always overrides the dispatcher when shallow rendering This is done so that any effects scheduled by the shallow render are thrown away. Unlike the code this was forked from (in ReactComponentStackFrame) DevTools should override the dispatcher even when DevTools is compiled in production mode, because the app itself may be in development mode and log errors/warnings. --- .../src/__tests__/componentStacks-test.js | 105 ++++++++++++++++++ .../backend/DevToolsComponentStackFrame.js | 24 ++-- 2 files changed, 117 insertions(+), 12 deletions(-) create mode 100644 packages/react-devtools-shared/src/__tests__/componentStacks-test.js diff --git a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js new file mode 100644 index 0000000000000..3028fc182da5d --- /dev/null +++ b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js @@ -0,0 +1,105 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +function normalizeCodeLocInfo(str) { + if (typeof str !== 'string') { + return str; + } + // This special case exists only for the special source location in + // ReactElementValidator. That will go away if we remove source locations. + str = str.replace(/Check your code at .+?:\d+/g, 'Check your code at **'); + // V8 format: + // at Component (/path/filename.js:123:45) + // React format: + // in Component (at filename.js:123) + return str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) { + return '\n in ' + name + ' (at **)'; + }); +} + +describe('component stack', () => { + let React; + let ReactDOM; + let act; + let mockError; + let mockWarn; + + beforeEach(() => { + // Intercept native console methods before DevTools bootstraps. + // Normalize component stack locations. + mockError = jest.fn(); + mockWarn = jest.fn(); + console.error = (...args) => { + mockError(...args.map(normalizeCodeLocInfo)); + }; + console.warn = (...args) => { + mockWarn(...args.map(normalizeCodeLocInfo)); + }; + + const utils = require('./utils'); + act = utils.act; + + React = require('react'); + ReactDOM = require('react-dom'); + }); + + it('should log the current component stack along with an error or warning', () => { + const Grandparent = () => ; + const Parent = () => ; + const Child = () => { + console.error('Test error.'); + console.warn('Test warning.'); + return null; + }; + + const container = document.createElement('div'); + + act(() => ReactDOM.render(, container)); + + expect(mockError).toHaveBeenCalledWith( + 'Test error.', + '\n in Child (at **)' + + '\n in Parent (at **)' + + '\n in Grandparent (at **)', + ); + expect(mockWarn).toHaveBeenCalledWith( + 'Test warning.', + '\n in Child (at **)' + + '\n in Parent (at **)' + + '\n in Grandparent (at **)', + ); + }); + + // This test should have caught #19911 + // but didn't because both DevTools and ReactDOM are running in the same memory space, + // so the case we're testing against (DevTools prod build and React DEV build) doesn't exist. + // It would be nice to figure out a way to test this combination at some point... + xit('should disable the current dispatcher before shallow rendering so no effects get scheduled', () => { + let useEffectCount = 0; + + const Example = props => { + React.useEffect(() => { + useEffectCount++; + expect(props).toBeDefined(); + }, [props]); + console.warn('Warning to trigger appended component stacks.'); + return null; + }; + + const container = document.createElement('div'); + act(() => ReactDOM.render(, container)); + + expect(useEffectCount).toBe(1); + + expect(mockWarn).toHaveBeenCalledWith( + 'Warning to trigger appended component stacks.', + '\n in Example (at **)', + ); + }); +}); diff --git a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js index b3b306a111be9..581fdfa40a1b8 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js +++ b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js @@ -85,14 +85,16 @@ export function describeNativeComponentFrame( Error.prepareStackTrace = undefined; reentry = true; - let previousDispatcher; - if (__DEV__) { - previousDispatcher = currentDispatcherRef.current; - // Set the dispatcher in DEV because this might be call in the render function - // for warnings. - currentDispatcherRef.current = null; - disableLogs(); - } + + // Override the dispatcher so effects scheduled by this shallow render are thrown away. + // + // Note that unlike the code this was forked from (in ReactComponentStackFrame) + // DevTools should override the dispatcher even when DevTools is compiled in production mode, + // because the app itself may be in development mode and log errors/warnings. + const previousDispatcher = currentDispatcherRef.current; + currentDispatcherRef.current = null; + disableLogs(); + try { // This should throw. if (construct) { @@ -188,10 +190,8 @@ export function describeNativeComponentFrame( Error.prepareStackTrace = previousPrepareStackTrace; - if (__DEV__) { - currentDispatcherRef.current = previousDispatcher; - reenableLogs(); - } + currentDispatcherRef.current = previousDispatcher; + reenableLogs(); } // Fallback to just using the name if we couldn't make it throw. const name = fn ? fn.displayName || fn.name : '';