@samitouri / QOS-React / commits / ecd919a2f9

RFC: warn when returning different hooks on subsequent renders (#14585)

* warn when returning different hooks on next render like it says. adds a field to Hook to track effect 'type', and compares when cloning subsequently. * lint * review changes - numbered enum for hook types - s/hookType/_debugType - better dce * cleaner detection location * redundant comments * different EffectHook / LayoutEffectHook * prettier * top level currentHookType * nulling currentHookType need to verify dce still works * small enhancements * hook order checks for useContext/useImperative * prettier * stray whitespace * move some bits around * better errors * pass tests * lint, flow * show a before - after diff * an error stack in the warning * lose currentHookMatches, fix a test * tidy * clear the mismatch only in dev * pass flow * side by side diff * tweak warning * pass flow * dedupe warnings per fiber, nits * better format * nit * fix bad merge, pass flow * lint * missing hooktype enum * merge currentHookType/currentHookNameInDev, fix nits * lint * final nits

Sunil Pai committed Jan 22, 2019 at 22:40 UTC ecd919a2f9b6e3d314fffc3d34b68b953a4e75e1

2 files changed +253 -18

packages/react-reconciler/src/ReactFiberHooks.js

+187 -18

@@ -54,6 +54,27 @@ type UpdateQueue<S, A> = {

54 eagerState: S | null,

55 };

57 +type HookType =

58 + | 'useState'

59 + | 'useReducer'

60 + | 'useContext'

61 + | 'useRef'

62 + | 'useEffect'

63 + | 'useLayoutEffect'

64 + | 'useCallback'

65 + | 'useMemo'

66 + | 'useImperativeHandle'

67 + | 'useDebugValue';

68 +

69 +// the first instance of a hook mismatch in a component,

70 +// represented by a portion of it's stacktrace

71 +let currentHookMismatch = null;

72 +

73 +let didWarnAboutMismatchedHooksForComponent;

74 +if (__DEV__) {

75 + didWarnAboutMismatchedHooksForComponent = new Set();

76 +}

77 +

78 export type Hook = {

79 memoizedState: any,

@@ -64,6 +85,10 @@ export type Hook = {

85 next: Hook | null,

86 };

88 +type HookDev = Hook & {

89 + _debugType: HookType,

90 +};

91 +

92 type Effect = {

93 tag: HookEffectTag,

94 create: () => mixed,

@@ -118,7 +143,7 @@ let numberOfReRenders: number = -1;

143 const RE_RENDER_LIMIT = 25;

144

145 // In DEV, this is the name of the currently executing primitive hook

121 -let currentHookNameInDev: ?string;

146 +let currentHookNameInDev: ?HookType = null;

147

148 function resolveCurrentlyRenderingFiber(): Fiber {

149 invariant(

@@ -170,6 +195,95 @@ function areHookInputsEqual(

195 return true;

196 }

197

198 +// till we have String::padEnd, a small function to

199 +// right-pad strings with spaces till a minimum length

200 +function padEndSpaces(string: string, length: number) {

201 + if (__DEV__) {

202 + if (string.length >= length) {

203 + return string;

204 + } else {

205 + return string + ' ' + new Array(length - string.length).join(' ');

206 + }

207 + }

208 +}

209 +

210 +function flushHookMismatchWarnings() {

211 + // we'll show the diff of the low level hooks,

212 + // and a stack trace so the dev can locate where

213 + // the first mismatch is coming from

214 + if (__DEV__) {

215 + if (currentHookMismatch !== null) {

216 + let componentName = getComponentName(

217 + ((currentlyRenderingFiber: any): Fiber).type,

218 + );

219 + if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) {

220 + didWarnAboutMismatchedHooksForComponent.add(componentName);

221 + const hookStackDiff = [];

222 + let current = firstCurrentHook;

223 + const previousOrder = [];

224 + while (current !== null) {

225 + previousOrder.push(((current: any): HookDev)._debugType);

226 + current = current.next;

227 + }

228 + let workInProgress = firstWorkInProgressHook;

229 + const nextOrder = [];

230 + while (workInProgress !== null) {

231 + nextOrder.push(((workInProgress: any): HookDev)._debugType);

232 + workInProgress = workInProgress.next;

233 + }

234 + // some bookkeeping for formatting the output table

235 + const columnLength = Math.max.apply(

236 + null,

237 + previousOrder

238 + .map(hook => hook.length)

239 + .concat(' Previous render'.length),

240 + );

241 +

242 + let hookStackHeader =

243 + ((padEndSpaces(' Previous render', columnLength): any): string) +

244 + ' Next render\n';

245 + const hookStackWidth = hookStackHeader.length;

246 + hookStackHeader += ' ' + new Array(hookStackWidth - 2).join('-');

247 + const hookStackFooter = ' ' + new Array(hookStackWidth - 2).join('^');

248 +

249 + const hookStackLength = Math.max(

250 + previousOrder.length,

251 + nextOrder.length,

252 + );

253 + for (let i = 0; i < hookStackLength; i++) {

254 + hookStackDiff.push(

255 + ((padEndSpaces(i + 1 + '. ', 3): any): string) +

256 + ((padEndSpaces(previousOrder[i], columnLength): any): string) +

257 + ' ' +

258 + nextOrder[i],

259 + );

260 + if (previousOrder[i] !== nextOrder[i]) {

261 + break;

262 + }

263 + }

264 + warning(

265 + false,

266 + 'React has detected a change in the order of Hooks called by %s. ' +

267 + 'This will lead to bugs and errors if not fixed. ' +

268 + 'For more information, read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' +

269 + '%s\n' +

270 + '%s\n' +

271 + '%s\n' +

272 + 'The first Hook type mismatch occured at:\n' +

273 + '%s\n\n' +

274 + 'This error occurred in the following component:',

275 + componentName,

276 + hookStackHeader,

277 + hookStackDiff.join('\n'),

278 + hookStackFooter,

279 + currentHookMismatch,

280 + );

281 + }

282 + currentHookMismatch = null;

283 + }

284 + }

285 +}

286 +

287 export function renderWithHooks(

288 current: Fiber | null,

289 workInProgress: Fiber,

@@ -221,6 +335,7 @@ export function renderWithHooks(

335 getComponentName(Component),

336 );

337 }

338 + flushHookMismatchWarnings();

339 }

340 } while (didScheduleRenderPhaseUpdate);

341

@@ -248,7 +363,7 @@ export function renderWithHooks(

363 componentUpdateQueue = null;

364

365 if (__DEV__) {

251 - currentHookNameInDev = undefined;

366 + currentHookNameInDev = null;

367 }

368

369 // These were reset above

@@ -281,6 +396,9 @@ export function resetHooks(): void {

396 if (!enableHooks) {

397 return;

398 }

399 + if (__DEV__) {

400 + flushHookMismatchWarnings();

401 + }

402

403 // This is used to reset the state of this module when a component throws.

404 // It's also called inside mountIndeterminateComponent if we determine the

@@ -297,7 +415,7 @@ export function resetHooks(): void {

415 componentUpdateQueue = null;

416

417 if (__DEV__) {

300 - currentHookNameInDev = undefined;

418 + currentHookNameInDev = null;

419 }

420

421 didScheduleRenderPhaseUpdate = false;

@@ -306,27 +424,63 @@ export function resetHooks(): void {

424 }

425

426 function createHook(): Hook {

309 - return {

310 - memoizedState: null,

427 + let hook: Hook = __DEV__

428 + ? {

429 + _debugType: ((currentHookNameInDev: any): HookType),

430 + memoizedState: null,

431

312 - baseState: null,

313 - queue: null,

314 - baseUpdate: null,

432 + baseState: null,

433 + queue: null,

434 + baseUpdate: null,

435

316 - next: null,

317 - };

436 + next: null,

437 + }

438 + : {

439 + memoizedState: null,

440 +

441 + baseState: null,

442 + queue: null,

443 + baseUpdate: null,

444 +

445 + next: null,

446 + };

447 +

448 + return hook;

449 }

450

451 function cloneHook(hook: Hook): Hook {

321 - return {

322 - memoizedState: hook.memoizedState,

452 + let nextHook: Hook = __DEV__

453 + ? {

454 + _debugType: ((currentHookNameInDev: any): HookType),

455 + memoizedState: hook.memoizedState,

456

324 - baseState: hook.baseState,

325 - queue: hook.queue,

326 - baseUpdate: hook.baseUpdate,

457 + baseState: hook.baseState,

458 + queue: hook.queue,

459 + baseUpdate: hook.baseUpdate,

460

328 - next: null,

329 - };

461 + next: null,

462 + }

463 + : {

464 + memoizedState: hook.memoizedState,

465 +

466 + baseState: hook.baseState,

467 + queue: hook.queue,

468 + baseUpdate: hook.baseUpdate,

469 +

470 + next: null,

471 + };

472 +

473 + if (__DEV__) {

474 + if (!currentHookMismatch) {

475 + if (currentHookNameInDev !== ((hook: any): HookDev)._debugType) {

476 + currentHookMismatch = new Error('tracer').stack

477 + .split('\n')

478 + .slice(4)

479 + .join('\n');

480 + }

481 + }

482 + }

483 + return nextHook;

484 }

485

486 function createWorkInProgressHook(): Hook {

@@ -390,6 +544,8 @@ export function useContext<T>(

544 ): T {

545 if (__DEV__) {

546 currentHookNameInDev = 'useContext';

547 + createWorkInProgressHook();

548 + currentHookNameInDev = null;

549 }

550 // Ensure we're in a function component (class components support only the

551 // .unstable_read() form)

@@ -422,6 +578,7 @@ export function useReducer<S, A>(

578 }

579 let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());

580 workInProgressHook = createWorkInProgressHook();

581 + currentHookNameInDev = null;

582 let queue: UpdateQueue<S, A> | null = (workInProgressHook.queue: any);

583 if (queue !== null) {

584 // Already have a queue, so this is an update.

@@ -600,7 +757,11 @@ function pushEffect(tag, create, destroy, deps) {

757

758 export function useRef<T>(initialValue: T): {current: T} {

759 currentlyRenderingFiber = resolveCurrentlyRenderingFiber();

760 + if (__DEV__) {

761 + currentHookNameInDev = 'useRef';

762 + }

763 workInProgressHook = createWorkInProgressHook();

764 + currentHookNameInDev = null;

765 let ref;

766

767 if (workInProgressHook.memoizedState === null) {

@@ -620,7 +781,9 @@ export function useLayoutEffect(

781 deps: Array<mixed> | void | null,

782 ): void {

783 if (__DEV__) {

623 - currentHookNameInDev = 'useLayoutEffect';

784 + if (currentHookNameInDev !== 'useImperativeHandle') {

785 + currentHookNameInDev = 'useLayoutEffect';

786 + }

787 }

788 useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);

789 }

@@ -653,6 +816,7 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {

816 const prevDeps = prevEffect.deps;

817 if (areHookInputsEqual(nextDeps, prevDeps)) {

818 pushEffect(NoHookEffect, create, destroy, nextDeps);

819 + currentHookNameInDev = null;

820 return;

821 }

822 }

@@ -665,6 +829,7 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {

829 destroy,

830 nextDeps,

831 );

832 + currentHookNameInDev = null;

833 }

834

835 export function useImperativeHandle<T>(

@@ -746,11 +911,13 @@ export function useCallback<T>(

911 if (nextDeps !== null) {

912 const prevDeps: Array<mixed> | null = prevState[1];

913 if (areHookInputsEqual(nextDeps, prevDeps)) {

914 + currentHookNameInDev = null;

915 return prevState[0];

916 }

917 }

918 }

919 workInProgressHook.memoizedState = [callback, nextDeps];

920 + currentHookNameInDev = null;

921 return callback;

922 }

923

@@ -772,6 +939,7 @@ export function useMemo<T>(

939 if (nextDeps !== null) {

940 const prevDeps: Array<mixed> | null = prevState[1];

941 if (areHookInputsEqual(nextDeps, prevDeps)) {

942 + currentHookNameInDev = null;

943 return prevState[0];

944 }

945 }

@@ -782,6 +950,7 @@ export function useMemo<T>(

950 const nextValue = nextCreate();

951 currentlyRenderingFiber = fiber;

952 workInProgressHook.memoizedState = [nextValue, nextDeps];

953 + currentHookNameInDev = null;

954 return nextValue;

955 }

956

packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js

+66

@@ -699,4 +699,70 @@ describe('ReactHooks', () => {

699 'Hooks can only be called inside the body of a function component',

700 );

701 });

702 +

703 + it('warns on using differently ordered hooks on subsequent renders', () => {

704 + const {useState, useReducer} = React;

705 + function useCustomHook() {

706 + return useState(0);

707 + }

708 + function App(props) {

709 + /* eslint-disable no-unused-vars */

710 + if (props.flip) {

711 + useCustomHook(0);

712 + useReducer((s, a) => a, 0);

713 + } else {

714 + useReducer((s, a) => a, 0);

715 + useCustomHook(0);

716 + }

717 + return null;

718 + /* eslint-enable no-unused-vars */

719 + }

720 + let root = ReactTestRenderer.create(<App flip={false} />);

721 + expect(() => {

722 + root.update(<App flip={true} />);

723 + }).toWarnDev([

724 + 'Warning: React has detected a change in the order of Hooks called by App. ' +

725 + 'This will lead to bugs and errors if not fixed. For more information, ' +

726 + 'read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' +

727 + ' Previous render Next render\n' +

728 + ' -------------------------------\n' +

729 + '1. useReducer useState\n' +

730 + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^',

731 + ]);

732 +

733 + // further warnings for this component are silenced

734 + root.update(<App flip={false} />);

735 + });

736 +

737 + it('detects a bad hook order even if the component throws', () => {

738 + const {useState, useReducer} = React;

739 + function useCustomHook() {

740 + useState(0);

741 + }

742 + function App(props) {

743 + /* eslint-disable no-unused-vars */

744 + if (props.flip) {

745 + useCustomHook();

746 + useReducer((s, a) => a, 0);

747 + throw new Error('custom error');

748 + } else {

749 + useReducer((s, a) => a, 0);

750 + useCustomHook();

751 + }

752 + return null;

753 + /* eslint-enable no-unused-vars */

754 + }

755 + let root = ReactTestRenderer.create(<App flip={false} />);

756 + expect(() => {

757 + expect(() => root.update(<App flip={true} />)).toThrow('custom error');

758 + }).toWarnDev([

759 + 'Warning: React has detected a change in the order of Hooks called by App. ' +

760 + 'This will lead to bugs and errors if not fixed. For more information, ' +

761 + 'read the Rules of Hooks: https://fb.me/rules-of-hooks\n\n' +

762 + ' Previous render Next render\n' +

763 + ' -------------------------------\n' +

764 + '1. useReducer useState\n' +

765 + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^',

766 + ]);

767 + });

768 });