From 67892afc7dfd3f0cdd3beea064ab3c46ca8434d5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 29 Jan 2019 17:28:35 -0800 Subject: [PATCH 1/2] Move Hook mismatch warning to first mismatch site Allows us to localize the warning logic in one place. --- .../react-reconciler/src/ReactFiberHooks.js | 163 +++++++----------- .../src/__tests__/ReactHooks-test.internal.js | 8 +- 2 files changed, 64 insertions(+), 107 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 2955a0e55e82a..77fab1abad9e2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -98,10 +98,6 @@ type HookType = | 'useImperativeHandle' | 'useDebugValue'; -// the first instance of a hook mismatch in a component, -// represented by a portion of its stacktrace -let currentHookMismatchInDev = null; - let didWarnAboutMismatchedHooksForComponent; if (__DEV__) { didWarnAboutMismatchedHooksForComponent = new Set(); @@ -180,6 +176,57 @@ const RE_RENDER_LIMIT = 25; // In DEV, this is the name of the currently executing primitive hook let currentHookNameInDev: ?HookType = null; +function warnOnHookMismatchInDev() { + if (__DEV__) { + const componentName = getComponentName( + ((currentlyRenderingFiber: any): Fiber).type, + ); + if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) { + didWarnAboutMismatchedHooksForComponent.add(componentName); + + let table = + ' Previous render Next render\n' + + ' -------------------------------\n'; + const secondColumnStart = 22; + + let prevHook: HookDev | null = (firstCurrentHook: any); + let nextHook: HookDev | null = (firstWorkInProgressHook: any); + let n = 1; + while (prevHook !== null && nextHook !== null) { + const oldHookName = prevHook._debugType; + const newHookName = nextHook._debugType; + + let row = `${n}. ${oldHookName}`; + + // Extra space so second column lines up + // lol @ IE not supporting String#repeat + while (row.length < secondColumnStart) { + row += ' '; + } + + row += newHookName + '\n'; + + table += row; + prevHook = (prevHook.next: any); + nextHook = (nextHook.next: any); + n++; + } + + table += ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^'; + + warning( + false, + 'React has detected a change in the order of Hooks called by %s. ' + + 'This will lead to bugs and errors if not fixed. ' + + 'For more information, read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' + + '%s\n', + componentName, + table, + ); + } + } +} + function throwInvalidHookError() { invariant( false, @@ -229,90 +276,6 @@ function areHookInputsEqual( return true; } -function flushHookMismatchWarnings() { - // we'll show the diff of the low level hooks, - // and a stack trace so the dev can locate where - // the first mismatch is coming from - if (__DEV__) { - if (currentHookMismatchInDev !== null) { - let componentName = getComponentName( - ((currentlyRenderingFiber: any): Fiber).type, - ); - if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) { - didWarnAboutMismatchedHooksForComponent.add(componentName); - const hookStackDiff = []; - let current = firstCurrentHook; - const previousOrder = []; - while (current !== null) { - previousOrder.push(((current: any): HookDev)._debugType); - current = current.next; - } - let workInProgress = firstWorkInProgressHook; - const nextOrder = []; - while (workInProgress !== null) { - nextOrder.push(((workInProgress: any): HookDev)._debugType); - workInProgress = workInProgress.next; - } - // some bookkeeping for formatting the output table - const columnLength = Math.max.apply( - null, - previousOrder - .map(hook => hook.length) - .concat(' Previous render'.length), - ); - - const padEndSpaces = (string, length) => { - if (string.length >= length) { - return string; - } - return string + ' ' + new Array(length - string.length).join(' '); - }; - - let hookStackHeader = - ((padEndSpaces(' Previous render', columnLength): any): string) + - ' Next render\n'; - const hookStackWidth = hookStackHeader.length; - hookStackHeader += ' ' + new Array(hookStackWidth - 2).join('-'); - const hookStackFooter = ' ' + new Array(hookStackWidth - 2).join('^'); - - const hookStackLength = Math.max( - previousOrder.length, - nextOrder.length, - ); - for (let i = 0; i < hookStackLength; i++) { - hookStackDiff.push( - ((padEndSpaces(i + 1 + '. ', 3): any): string) + - ((padEndSpaces(previousOrder[i], columnLength): any): string) + - ' ' + - nextOrder[i], - ); - if (previousOrder[i] !== nextOrder[i]) { - break; - } - } - warning( - false, - 'React has detected a change in the order of Hooks called by %s. ' + - 'This will lead to bugs and errors if not fixed. ' + - 'For more information, read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' + - '%s\n' + - '%s\n' + - '%s\n' + - 'The first Hook type mismatch occured at:\n' + - '%s\n\n' + - 'This error occurred in the following component:', - componentName, - hookStackHeader, - hookStackDiff.join('\n'), - hookStackFooter, - currentHookMismatchInDev, - ); - } - currentHookMismatchInDev = null; - } - } -} - export function renderWithHooks( current: Fiber | null, workInProgress: Fiber, @@ -378,7 +341,6 @@ export function renderWithHooks( } if (__DEV__) { - flushHookMismatchWarnings(); currentHookNameInDev = null; } @@ -437,10 +399,6 @@ export function bailoutHooks( } export function resetHooks(): void { - if (__DEV__) { - flushHookMismatchWarnings(); - } - // We can assume the previous dispatcher is always this one, since we set it // at the beginning of the render phase and there's no re-entrancy. ReactCurrentDispatcher.current = ContextOnlyDispatcher; @@ -537,18 +495,6 @@ function updateWorkInProgressHook(): Hook { next: null, }; - if (__DEV__) { - (newHook: any)._debugType = (currentHookNameInDev: any); - if (currentHookMismatchInDev === null) { - if (currentHookNameInDev !== ((currentHook: any): HookDev)._debugType) { - currentHookMismatchInDev = new Error('tracer').stack - .split('\n') - .slice(4) - .join('\n'); - } - } - } - if (workInProgressHook === null) { // This is the first hook in the list. workInProgressHook = firstWorkInProgressHook = newHook; @@ -557,6 +503,13 @@ function updateWorkInProgressHook(): Hook { workInProgressHook = workInProgressHook.next = newHook; } nextCurrentHook = currentHook.next; + + if (__DEV__) { + (newHook: any)._debugType = (currentHookNameInDev: any); + if (currentHookNameInDev !== ((currentHook: any): HookDev)._debugType) { + warnOnHookMismatchInDev(); + } + } } return workInProgressHook; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 0a37db0d37b89..8c46ef9215af9 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1159,7 +1159,7 @@ describe('ReactHooks', () => { }); it('warns on using differently ordered hooks on subsequent renders', () => { - const {useState, useReducer} = React; + const {useState, useReducer, useRef} = React; function useCustomHook() { return useState(0); } @@ -1172,6 +1172,9 @@ describe('ReactHooks', () => { useReducer((s, a) => a, 0); useCustomHook(0); } + // This should not appear in the warning message because it occurs after + // the first mismatch + const ref = useRef(null); return null; /* eslint-enable no-unused-vars */ } @@ -1185,7 +1188,8 @@ describe('ReactHooks', () => { ' Previous render Next render\n' + ' -------------------------------\n' + '1. useReducer useState\n' + - ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^', + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n' + + ' in App (at **)', ]); // further warnings for this component are silenced From abde12b2114526d5a784d37ba9ec41a2dee2d845 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 29 Jan 2019 12:49:14 -0800 Subject: [PATCH 2/2] Nit --- packages/react-reconciler/src/ReactFiberHooks.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 77fab1abad9e2..8312fb0a8094c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -184,11 +184,9 @@ function warnOnHookMismatchInDev() { if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) { didWarnAboutMismatchedHooksForComponent.add(componentName); - let table = - ' Previous render Next render\n' + - ' -------------------------------\n'; const secondColumnStart = 22; + let table = ''; let prevHook: HookDev | null = (firstCurrentHook: any); let nextHook: HookDev | null = (firstWorkInProgressHook: any); let n = 1; @@ -212,14 +210,15 @@ function warnOnHookMismatchInDev() { n++; } - table += ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^'; - warning( false, 'React has detected a change in the order of Hooks called by %s. ' + 'This will lead to bugs and errors if not fixed. ' + 'For more information, read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' + - '%s\n', + ' Previous render Next render\n' + + ' -------------------------------\n' + + '%s' + + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n', componentName, table, );