From 6470e0f169b4cf0416132aa66221bf20e264c618 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 25 Nov 2019 17:25:34 +0000 Subject: [PATCH] [Fresh] Make all errors recoverable (#17438) * [Fresh] Detect root updates more reliably * [Fresh] Use WeakMap for root elements * [Fresh] Make initial failures recoverable too * Fix DevTools check * Fix wrong flow type --- .../src/ReactFiberDevToolsHook.js | 27 +++++ .../src/ReactFiberInstrumentation.js | 18 --- .../src/ReactFiberReconciler.js | 18 +-- .../react-refresh/src/ReactFreshRuntime.js | 99 ++++++++++----- .../src/__tests__/ReactFresh-test.js | 113 +++++++++++++++--- 5 files changed, 195 insertions(+), 80 deletions(-) delete mode 100644 packages/react-reconciler/src/ReactFiberInstrumentation.js diff --git a/packages/react-reconciler/src/ReactFiberDevToolsHook.js b/packages/react-reconciler/src/ReactFiberDevToolsHook.js index af61b3bb65b6..426a9172b362 100644 --- a/packages/react-reconciler/src/ReactFiberDevToolsHook.js +++ b/packages/react-reconciler/src/ReactFiberDevToolsHook.js @@ -14,12 +14,14 @@ import {inferPriorityFromExpirationTime} from './ReactFiberExpirationTime'; import type {Fiber} from './ReactFiber'; import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {ReactNodeList} from 'shared/ReactTypes'; import {DidCapture} from 'shared/ReactSideEffectTags'; import warningWithoutStack from 'shared/warningWithoutStack'; declare var __REACT_DEVTOOLS_GLOBAL_HOOK__: Object | void; +let onScheduleFiberRoot = null; let onCommitFiberRoot = null; let onCommitFiberUnmount = null; let hasLoggedError = false; @@ -54,6 +56,25 @@ export function injectInternals(internals: Object): boolean { try { const rendererID = hook.inject(internals); // We have successfully injected, so now it is safe to set up hooks. + if (__DEV__) { + // Only used by Fast Refresh + if (typeof hook.onScheduleFiberRoot === 'function') { + onScheduleFiberRoot = (root, children) => { + try { + hook.onScheduleFiberRoot(rendererID, root, children); + } catch (err) { + if (__DEV__ && !hasLoggedError) { + hasLoggedError = true; + warningWithoutStack( + false, + 'React DevTools encountered an error: %s', + err, + ); + } + } + }; + } + } onCommitFiberRoot = (root, expirationTime) => { try { const didError = (root.current.effectTag & DidCapture) === DidCapture; @@ -106,6 +127,12 @@ export function injectInternals(internals: Object): boolean { return true; } +export function onScheduleRoot(root: FiberRoot, children: ReactNodeList) { + if (typeof onScheduleFiberRoot === 'function') { + onScheduleFiberRoot(root, children); + } +} + export function onCommitRoot(root: FiberRoot, expirationTime: ExpirationTime) { if (typeof onCommitFiberRoot === 'function') { onCommitFiberRoot(root, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberInstrumentation.js b/packages/react-reconciler/src/ReactFiberInstrumentation.js deleted file mode 100644 index 3084de928c96..000000000000 --- a/packages/react-reconciler/src/ReactFiberInstrumentation.js +++ /dev/null @@ -1,18 +0,0 @@ -/** - * 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 - */ - -// This lets us hook into Fiber to debug what it's doing. -// See https://github.com/facebook/react/pull/8033. -// This is not part of the public API, not even for React DevTools. -// You may only inject a debugTool if you work on React Fiber itself. -const ReactFiberInstrumentation = { - debugTool: null, -}; - -module.exports = ReactFiberInstrumentation; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index d70196c55bfe..6ebac0dace80 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -48,7 +48,7 @@ import { isContextProvider as isLegacyContextProvider, } from './ReactFiberContext'; import {createFiberRoot} from './ReactFiberRoot'; -import {injectInternals} from './ReactFiberDevToolsHook'; +import {injectInternals, onScheduleRoot} from './ReactFiberDevToolsHook'; import { requestCurrentTimeForUpdate, computeExpirationForFiber, @@ -69,7 +69,6 @@ import { IsThisRendererActing, } from './ReactFiberWorkLoop'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; -import ReactFiberInstrumentation from './ReactFiberInstrumentation'; import { getStackByFiberInDevAndProd, phase as ReactCurrentFiberPhase, @@ -230,6 +229,9 @@ export function updateContainer( parentComponent: ?React$Component, callback: ?Function, ): ExpirationTime { + if (__DEV__) { + onScheduleRoot(container, element); + } const current = container.current; const currentTime = requestCurrentTimeForUpdate(); if (__DEV__) { @@ -246,18 +248,6 @@ export function updateContainer( suspenseConfig, ); - if (__DEV__) { - if (ReactFiberInstrumentation.debugTool) { - if (current.alternate === null) { - ReactFiberInstrumentation.debugTool.onMountContainer(container); - } else if (element === null) { - ReactFiberInstrumentation.debugTool.onUnmountContainer(container); - } else { - ReactFiberInstrumentation.debugTool.onUpdateContainer(container); - } - } - } - const context = getContextForSubtree(parentComponent); if (container.context === null) { container.context = context; diff --git a/packages/react-refresh/src/ReactFreshRuntime.js b/packages/react-refresh/src/ReactFreshRuntime.js index 65a8f73657e1..df8c134c6c1d 100644 --- a/packages/react-refresh/src/ReactFreshRuntime.js +++ b/packages/react-refresh/src/ReactFreshRuntime.js @@ -68,9 +68,17 @@ let helpersByRoot: Map = new Map(); // We keep track of mounted roots so we can schedule updates. let mountedRoots: Set = new Set(); -// If a root captures an error, we add its element to this Map so we can retry on edit. -let failedRoots: Map = new Map(); -let didSomeRootFailOnMount = false; +// If a root captures an error, we remember it so we can retry on edit. +let failedRoots: Set = new Set(); + +// In environments that support WeakMap, we also remember the last element for every root. +// It needs to be weak because we do this even for roots that failed to mount. +// If there is no WeakMap, we won't attempt to do retrying. +// $FlowIssue +let rootElements: WeakMap | null = // $FlowIssue + typeof WeakMap === 'function' ? new WeakMap() : null; + +let isPerformingRefresh = false; function computeFullKey(signature: Signature): string { if (signature.fullKey !== null) { @@ -171,11 +179,20 @@ function cloneSet(set: Set): Set { } export function performReactRefresh(): RefreshUpdate | null { - if (__DEV__) { - if (pendingUpdates.length === 0) { - return null; - } + if (!__DEV__) { + throw new Error( + 'Unexpected call to React Refresh in a production environment.', + ); + } + if (pendingUpdates.length === 0) { + return null; + } + if (isPerformingRefresh) { + return null; + } + isPerformingRefresh = true; + try { const staleFamilies = new Set(); const updatedFamilies = new Set(); @@ -216,17 +233,27 @@ export function performReactRefresh(): RefreshUpdate | null { // If we don't do this, there is a risk they will be mutated while // we iterate over them. For example, trying to recover a failed root // may cause another root to be added to the failed list -- an infinite loop. - let failedRootsSnapshot = cloneMap(failedRoots); + let failedRootsSnapshot = cloneSet(failedRoots); let mountedRootsSnapshot = cloneSet(mountedRoots); let helpersByRootSnapshot = cloneMap(helpersByRoot); - failedRootsSnapshot.forEach((element, root) => { + failedRootsSnapshot.forEach(root => { const helpers = helpersByRootSnapshot.get(root); if (helpers === undefined) { throw new Error( 'Could not find helpers for a root. This is a bug in React Refresh.', ); } + if (!failedRoots.has(root)) { + // No longer failed. + } + if (rootElements === null) { + return; + } + if (!rootElements.has(root)) { + return; + } + const element = rootElements.get(root); try { helpers.scheduleRoot(root, element); } catch (err) { @@ -244,6 +271,9 @@ export function performReactRefresh(): RefreshUpdate | null { 'Could not find helpers for a root. This is a bug in React Refresh.', ); } + if (!mountedRoots.has(root)) { + // No longer mounted. + } try { helpers.scheduleRefresh(root, update); } catch (err) { @@ -258,10 +288,8 @@ export function performReactRefresh(): RefreshUpdate | null { throw firstError; } return update; - } else { - throw new Error( - 'Unexpected call to React Refresh in a production environment.', - ); + } finally { + isPerformingRefresh = false; } } @@ -411,6 +439,11 @@ export function injectIntoGlobalHook(globalObject: any): void { inject(injected) { return nextID++; }, + onScheduleFiberRoot( + id: number, + root: FiberRoot, + children: ReactNodeList, + ) {}, onCommitFiberRoot( id: number, root: FiberRoot, @@ -437,6 +470,22 @@ export function injectIntoGlobalHook(globalObject: any): void { // We also want to track currently mounted roots. const oldOnCommitFiberRoot = hook.onCommitFiberRoot; + const oldOnScheduleFiberRoot = hook.onScheduleFiberRoot || (() => {}); + hook.onScheduleFiberRoot = function( + id: number, + root: FiberRoot, + children: mixed, + ) { + if (!isPerformingRefresh) { + // If it was intentionally scheduled, don't attempt to restore. + // This includes intentionally scheduled unmounts. + failedRoots.delete(root); + if (rootElements !== null) { + rootElements.set(root, children); + } + } + return oldOnScheduleFiberRoot.apply(this, arguments); + }; hook.onCommitFiberRoot = function( id: number, root: FiberRoot, @@ -476,27 +525,14 @@ export function injectIntoGlobalHook(globalObject: any): void { mountedRoots.delete(root); if (didError) { // We'll remount it on future edits. - // Remember what was rendered so we can restore it. - failedRoots.set(root, alternate.memoizedState.element); + failedRoots.add(root); } else { helpersByRoot.delete(root); } } else if (!wasMounted && !isMounted) { - if (didError && !failedRoots.has(root)) { - // The root had an error during the initial mount. - // We can't read its last element from the memoized state - // because there was no previously committed alternate. - // Ideally, it would be nice if we had a way to extract - // the last attempted rendered element, but accessing the update queue - // would tie this package too closely to the reconciler version. - // So instead, we just set a flag. - // TODO: Maybe we could fix this as the same time as when we fix - // DevTools to not depend on `alternate.memoizedState.element`. - didSomeRootFailOnMount = true; - } else if (!didError && failedRoots.has(root)) { - // The error is fixed but the component is still unmounted. - // This means that the unmount was not caused by a failed refresh. - failedRoots.delete(root); + if (didError) { + // We'll remount it on future edits. + failedRoots.add(root); } } } else { @@ -514,7 +550,8 @@ export function injectIntoGlobalHook(globalObject: any): void { } export function hasUnrecoverableErrors() { - return didSomeRootFailOnMount; + // TODO: delete this after removing dependency in RN. + return false; } // Exposed for testing. diff --git a/packages/react-refresh/src/__tests__/ReactFresh-test.js b/packages/react-refresh/src/__tests__/ReactFresh-test.js index 046cd2866901..419284fd40ca 100644 --- a/packages/react-refresh/src/__tests__/ReactFresh-test.js +++ b/packages/react-refresh/src/__tests__/ReactFresh-test.js @@ -2755,11 +2755,8 @@ describe('ReactFresh', () => { } }); - // TODO: we can make this recoverable in the future - // if we add a way to track the last attempted element. - it('records an unrecoverable error if a root fails on mount', () => { + it('remounts a failed root on mount', () => { if (__DEV__) { - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); expect(() => { render(() => { function Hello() { @@ -2770,13 +2767,106 @@ describe('ReactFresh', () => { return Hello; }); }).toThrow('No'); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(true); + expect(container.innerHTML).toBe(''); + + // A bad retry + expect(() => { + patch(() => { + function Hello() { + throw new Error('Not yet'); + } + $RefreshReg$(Hello, 'Hello'); + }); + }).toThrow('Not yet'); + expect(container.innerHTML).toBe(''); + + // Perform a hot update that fixes the error. + patch(() => { + function Hello() { + return

Fixed!

; + } + $RefreshReg$(Hello, 'Hello'); + }); + // This should mount the root. + expect(container.innerHTML).toBe('

Fixed!

'); + + // Ensure we can keep failing and recovering later. + expect(() => { + patch(() => { + function Hello() { + throw new Error('No 2'); + } + $RefreshReg$(Hello, 'Hello'); + }); + }).toThrow('No 2'); + expect(container.innerHTML).toBe(''); + expect(() => { + patch(() => { + function Hello() { + throw new Error('Not yet 2'); + } + $RefreshReg$(Hello, 'Hello'); + }); + }).toThrow('Not yet 2'); + expect(container.innerHTML).toBe(''); + patch(() => { + function Hello() { + return

Fixed 2!

; + } + $RefreshReg$(Hello, 'Hello'); + }); + expect(container.innerHTML).toBe('

Fixed 2!

'); + + // Updates after intentional unmount are ignored. + ReactDOM.unmountComponentAtNode(container); + patch(() => { + function Hello() { + throw new Error('Ignored'); + } + $RefreshReg$(Hello, 'Hello'); + }); + expect(container.innerHTML).toBe(''); + patch(() => { + function Hello() { + return

Ignored

; + } + $RefreshReg$(Hello, 'Hello'); + }); + expect(container.innerHTML).toBe(''); + } + }); + + it('does not retry an intentionally unmounted failed root', () => { + if (__DEV__) { + expect(() => { + render(() => { + function Hello() { + throw new Error('No'); + } + $RefreshReg$(Hello, 'Hello'); + + return Hello; + }); + }).toThrow('No'); + expect(container.innerHTML).toBe(''); + + // Intentional unmount. + ReactDOM.unmountComponentAtNode(container); + + // Perform a hot update that fixes the error. + patch(() => { + function Hello() { + return

Fixed!

; + } + $RefreshReg$(Hello, 'Hello'); + }); + // This should stay unmounted. + expect(container.innerHTML).toBe(''); } }); it('remounts a failed root on update', () => { if (__DEV__) { - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); render(() => { function Hello() { return

Hi

; @@ -2786,7 +2876,6 @@ describe('ReactFresh', () => { return Hello; }); expect(container.innerHTML).toBe('

Hi

'); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Perform a hot update that fails. // This removes the root. @@ -2799,7 +2888,6 @@ describe('ReactFresh', () => { }); }).toThrow('No'); expect(container.innerHTML).toBe(''); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // A bad retry expect(() => { @@ -2811,7 +2899,6 @@ describe('ReactFresh', () => { }); }).toThrow('Not yet'); expect(container.innerHTML).toBe(''); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Perform a hot update that fixes the error. patch(() => { @@ -2822,7 +2909,6 @@ describe('ReactFresh', () => { }); // This should remount the root. expect(container.innerHTML).toBe('

Fixed!

'); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Verify next hot reload doesn't remount anything. let helloNode = container.firstChild; @@ -2834,7 +2920,6 @@ describe('ReactFresh', () => { }); expect(container.firstChild).toBe(helloNode); expect(helloNode.textContent).toBe('Nice.'); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Break again. expect(() => { @@ -2846,7 +2931,6 @@ describe('ReactFresh', () => { }); }).toThrow('Oops'); expect(container.innerHTML).toBe(''); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Perform a hot update that fixes the error. patch(() => { @@ -2857,7 +2941,6 @@ describe('ReactFresh', () => { }); // This should remount the root. expect(container.innerHTML).toBe('

At last.

'); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Check we don't attempt to reverse an intentional unmount. ReactDOM.unmountComponentAtNode(container); @@ -2869,7 +2952,6 @@ describe('ReactFresh', () => { $RefreshReg$(Hello, 'Hello'); }); expect(container.innerHTML).toBe(''); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Mount a new container. render(() => { @@ -2881,7 +2963,6 @@ describe('ReactFresh', () => { return Hello; }); expect(container.innerHTML).toBe('

Hi

'); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Break again. expect(() => { @@ -2893,7 +2974,6 @@ describe('ReactFresh', () => { }); }).toThrow('Oops'); expect(container.innerHTML).toBe(''); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); // Check we don't attempt to reverse an intentional unmount, even after an error. ReactDOM.unmountComponentAtNode(container); @@ -2905,7 +2985,6 @@ describe('ReactFresh', () => { $RefreshReg$(Hello, 'Hello'); }); expect(container.innerHTML).toBe(''); - expect(ReactFreshRuntime.hasUnrecoverableErrors()).toBe(false); } });