From df7ee2012b26ebb0a36ef96381565c15e0bdcd31 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 13 May 2021 16:53:19 -0400 Subject: [PATCH] DevTools: Fix Fiber leak caused by Refresh hot reloading --- .../FastRefreshDevToolsIntegration-test.js | 236 ++++++++++++++++++ .../src/backend/legacy/renderer.js | 6 + .../src/backend/renderer.js | 67 ++++- .../src/backend/types.js | 8 + packages/react-devtools-shared/src/hook.js | 8 + .../src/ReactFiberBeginWork.new.js | 30 ++- .../src/ReactFiberBeginWork.old.js | 30 ++- .../src/ReactFiberDevToolsHook.new.js | 25 ++ .../src/ReactFiberDevToolsHook.old.js | 25 ++ .../src/ReactFiberHotReloading.new.js | 1 + .../src/ReactFiberHotReloading.old.js | 1 + 11 files changed, 408 insertions(+), 29 deletions(-) create mode 100644 packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js diff --git a/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js b/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js new file mode 100644 index 0000000000000..279390c5fca8b --- /dev/null +++ b/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js @@ -0,0 +1,236 @@ +/** + * 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 + */ + +describe('Fast Refresh', () => { + let React; + let ReactDOM; + let ReactFreshRuntime; + let act; + let babel; + let container; + let exportsObj; + let freshPlugin; + let store; + let withErrorsOrWarningsIgnored; + + afterEach(() => { + jest.resetModules(); + }); + + beforeEach(() => { + exportsObj = undefined; + container = document.createElement('div'); + + babel = require('@babel/core'); + freshPlugin = require('react-refresh/babel'); + + store = global.store; + + React = require('react'); + + ReactFreshRuntime = require('react-refresh/runtime'); + ReactFreshRuntime.injectIntoGlobalHook(global); + + ReactDOM = require('react-dom'); + + const utils = require('./utils'); + act = utils.act; + withErrorsOrWarningsIgnored = utils.withErrorsOrWarningsIgnored; + }); + + function execute(source) { + const compiled = babel.transform(source, { + babelrc: false, + presets: ['@babel/react'], + plugins: [ + [freshPlugin, {skipEnvCheck: true}], + '@babel/plugin-transform-modules-commonjs', + '@babel/plugin-transform-destructuring', + ].filter(Boolean), + }).code; + exportsObj = {}; + // eslint-disable-next-line no-new-func + new Function( + 'global', + 'React', + 'exports', + '$RefreshReg$', + '$RefreshSig$', + compiled, + )(global, React, exportsObj, $RefreshReg$, $RefreshSig$); + // Module systems will register exports as a fallback. + // This is useful for cases when e.g. a class is exported, + // and we don't want to propagate the update beyond this module. + $RefreshReg$(exportsObj.default, 'exports.default'); + return exportsObj.default; + } + + function render(source) { + const Component = execute(source); + act(() => { + ReactDOM.render(, container); + }); + // Module initialization shouldn't be counted as a hot update. + expect(ReactFreshRuntime.performReactRefresh()).toBe(null); + } + + function patch(source) { + const prevExports = exportsObj; + execute(source); + const nextExports = exportsObj; + + // Check if exported families have changed. + // (In a real module system we'd do this for *all* exports.) + // For example, this can happen if you convert a class to a function. + // Or if you wrap something in a HOC. + const didExportsChange = + ReactFreshRuntime.getFamilyByType(prevExports.default) !== + ReactFreshRuntime.getFamilyByType(nextExports.default); + if (didExportsChange) { + // In a real module system, we would propagate such updates upwards, + // and re-execute modules that imported this one. (Just like if we edited them.) + // This makes adding/removing/renaming exports re-render references to them. + // Here, we'll just force a re-render using the newer type to emulate this. + const NextComponent = nextExports.default; + act(() => { + ReactDOM.render(, container); + }); + } + act(() => { + const result = ReactFreshRuntime.performReactRefresh(); + if (!didExportsChange) { + // Normally we expect that some components got updated in our tests. + expect(result).not.toBe(null); + } else { + // However, we have tests where we convert functions to classes, + // and in those cases it's expected nothing would get updated. + // (Instead, the export change branch above would take care of it.) + } + }); + expect(ReactFreshRuntime._getMountedRootCount()).toBe(1); + } + + function $RefreshReg$(type, id) { + ReactFreshRuntime.register(type, id); + } + + function $RefreshSig$() { + return ReactFreshRuntime.createSignatureFunctionForTransform(); + } + + it('should not break the DevTools store', () => { + render(` + function Parent() { + return ; + }; + + function Child() { + return null; + }; + + export default Parent; + `); + + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + `); + + patch(` + function Parent() { + return ; + }; + + function Child() { + return null; + }; + + export default Parent; + `); + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + `); + }); + + it('should not break when there are warnings in between patching', () => { + withErrorsOrWarningsIgnored(['Expected warning during render'], () => { + render(` + const {useState} = React; + + export default function Component() { + const [state, setState] = useState(1); + + console.warn("Expected warning during render"); + + return
; + } + `); + }); + expect(store).toMatchInlineSnapshot(` + ✕ 0, ⚠ 1 + [root] + ⚠ + `); + + let element = container.firstChild; + + withErrorsOrWarningsIgnored(['Expected warning during render'], () => { + patch(` + const {useState} = React; + + export default function Component() { + const [state, setState] = useState(1); + + console.warn("Expected warning during render"); + + return
; + } + `); + }); + + // This is the same component type, so the warning count carries over. + expect(store).toMatchInlineSnapshot(` + ✕ 0, ⚠ 2 + [root] + ⚠ + `); + + // State is preserved; this verifies that Fast Refresh is wired up. + expect(container.firstChild).toBe(element); + element = container.firstChild; + + withErrorsOrWarningsIgnored(['Expected warning during render'], () => { + patch(` + const {useEffect, useState} = React; + + export default function Component() { + const [state, setState] = useState(3); + useEffect(() => {}); + + console.warn("Expected warning during render"); + + return
; + } + `); + }); + + // This is a new component type, so the warning count has been reset. + expect(store).toMatchInlineSnapshot(` + ✕ 0, ⚠ 1 + [root] + ⚠ + `); + + // State is reset because hooks changed. + expect(container.firstChild).not.toBe(element); + }); +}); diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 091d755009b42..f4d3def1766b6 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -1007,6 +1007,11 @@ export function attach( const getProfilingData = () => { throw new Error('getProfilingData not supported by this renderer'); }; + const handleClonedForForceRemount = () => { + throw new Error( + 'handleClonedForForceRemount not supported by this renderer', + ); + }; const handleCommitFiberRoot = () => { throw new Error('handleCommitFiberRoot not supported by this renderer'); }; @@ -1084,6 +1089,7 @@ export function attach( getOwnersList, getPathForElement, getProfilingData, + handleClonedForForceRemount, handleCommitFiberRoot, handleCommitFiberUnmount, handlePostCommitFiberRoot, diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 3afbbf3dd4435..476a1781e0791 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -638,6 +638,10 @@ export function attach( // We should clean up Fibers like this when flushing; see recordPendingErrorsAndWarnings(). const fiberID = getFiberID(getPrimaryFiber(fiber)); + if (__DEBUG__) { + debug('onErrorOrWarning', fiber, null, `${type}: "${message}"`); + } + // Mark this Fiber as needed its warning/error count updated during the next flush. fibersWithChangedErrorOrWarningCounts.add(fiberID); @@ -702,16 +706,15 @@ export function attach( if (__DEBUG__) { const displayName = fiber.tag + ':' + (getDisplayNameForFiber(fiber) || 'null'); - const id = getFiberID(fiber); + + const id = getFiberIDSafe(fiber) || '-'; const parentDisplayName = parentFiber ? parentFiber.tag + ':' + (getDisplayNameForFiber(parentFiber) || 'null') : ''; - const parentID = parentFiber ? getFiberID(parentFiber) : ''; - // NOTE: calling getFiberID or getPrimaryFiber is unsafe here - // because it will put them in the map. For now, we'll omit them. - // TODO: better debugging story for this. + const parentID = parentFiber ? getFiberIDSafe(parentFiber) || '-' : ''; + console.log( `[renderer] %c${name} %c${displayName} (${id}) %c${ parentFiber ? `${parentDisplayName} (${parentID})` : '' @@ -979,6 +982,23 @@ export function attach( // When a mount or update is in progress, this value tracks the root that is being operated on. let currentRootID: number = -1; + function getFiberIDSafe(fiber: Fiber): number | null { + let primaryFiber = null; + if (primaryFibers.has(fiber)) { + primaryFiber = fiber; + } + const {alternate} = fiber; + if (alternate != null && primaryFibers.has(alternate)) { + primaryFiber = alternate; + } + + if (primaryFiber && fiberToIDMap.has(primaryFiber)) { + return ((fiberToIDMap.get(primaryFiber): any): number); + } + + return null; + } + function getFiberID(primaryFiber: Fiber): number { if (!fiberToIDMap.has(primaryFiber)) { const id = getUID(); @@ -1641,6 +1661,7 @@ export function attach( pendingRealUnmountedIDs.push(id); } } + fiberToIDMap.delete(primaryFiber); idToFiberMap.delete(id); primaryFibers.delete(primaryFiber); @@ -3815,6 +3836,41 @@ export function attach( traceUpdatesEnabled = isEnabled; } + function handleClonedForForceRemount(oldFiber: Fiber, newFiber: Fiber): void { + if (__DEBUG__) { + console.log( + '[renderer] handleClonedForForceRemount', + getDisplayNameForFiber(oldFiber), + '(' + getFiberID(getPrimaryFiber(oldFiber)), + '->', + getFiberID(getPrimaryFiber(newFiber)) + ')', + ); + } + + let primaryFiber = null; + if (primaryFibers.has(oldFiber)) { + primaryFiber = oldFiber; + } + const {alternate} = oldFiber; + if (alternate != null && primaryFibers.has(alternate)) { + primaryFiber = alternate; + } + + // Fast Refresh is about to (synchronously) clone and replace this part of the tree. + // In order to avoid these Fibers from leaking on the DevTools side, + // and possibly remaining visible in the Components tab as well, + // queue up unmount operations to send on the next commit. + if (primaryFiber) { + recordUnmount(primaryFiber, false); + unmountFiberChildrenRecursively(primaryFiber); + + const fiberID = ((fiberToIDMap.get(primaryFiber): any): number); + fiberToIDMap.delete(primaryFiber); + idToFiberMap.delete(fiberID); + primaryFibers.delete(primaryFiber); + } + } + return { cleanup, clearErrorsAndWarnings, @@ -3831,6 +3887,7 @@ export function attach( getOwnersList, getPathForElement, getProfilingData, + handleClonedForForceRemount, handleCommitFiberRoot, handleCommitFiberUnmount, handlePostCommitFiberRoot, diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 342931c250eab..5fc0950791e46 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -323,6 +323,7 @@ export type RendererInterface = { getProfilingData(): ProfilingDataBackend, getOwnersList: (id: number) => Array | null, getPathForElement: (id: number) => Array | null, + handleClonedForForceRemount: (oldFiber: Fiber, newFiber: Fiber) => void, handleCommitFiberRoot: (fiber: Object, commitPriority?: number) => void, handleCommitFiberUnmount: (fiber: Object) => void, handlePostCommitFiberRoot: (fiber: Object) => void, @@ -396,5 +397,12 @@ export type DevToolsHook = { // Added in v16.9 to support Fast Refresh didError?: boolean, ) => void, + + // Added in v17.x to improve Fast Refresh + DevTools integration + onClonedForForceRemount: ( + rendererID: RendererID, + oldFiber: Fiber, + newFiber: Fiber, + ) => void, ... }; diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index a4724f6f6b92b..d636a683d526b 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -261,6 +261,13 @@ export function installHook(target: any): DevToolsHook | null { return roots[rendererID]; } + function onClonedForForceRemount(rendererID, oldFiber, newFiber) { + const rendererInterface = rendererInterfaces.get(rendererID); + if (rendererInterface != null) { + rendererInterface.handleClonedForForceRemount(oldFiber, newFiber); + } + } + function onCommitFiberUnmount(rendererID, fiber) { const rendererInterface = rendererInterfaces.get(rendererID); if (rendererInterface != null) { @@ -306,6 +313,7 @@ export function installHook(target: any): DevToolsHook | null { // Fast Refresh for web relies on this. renderers, + onClonedForForceRemount, emit, getFiberRoots, diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 2dfc6766577f9..44dc1caefeeb6 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -32,6 +32,10 @@ import type {UpdateQueue} from './ReactUpdateQueue.new'; import checkPropTypes from 'shared/checkPropTypes'; +import { + isDevToolsPresent, + onClonedForForceRemount, +} from './ReactFiberDevToolsHook.new'; import { IndeterminateComponent, FunctionComponent, @@ -3194,19 +3198,21 @@ function beginWork( if (__DEV__) { if (workInProgress._debugNeedsRemount && current !== null) { - // This will restart the begin phase with a new fiber. - return remountFiber( - current, - workInProgress, - createFiberFromTypeAndProps( - workInProgress.type, - workInProgress.key, - workInProgress.pendingProps, - workInProgress._debugOwner || null, - workInProgress.mode, - workInProgress.lanes, - ), + const clonedWorkInProgress = createFiberFromTypeAndProps( + workInProgress.type, + workInProgress.key, + workInProgress.pendingProps, + workInProgress._debugOwner || null, + workInProgress.mode, + workInProgress.lanes, ); + + if (isDevToolsPresent) { + onClonedForForceRemount(workInProgress, clonedWorkInProgress); + } + + // This will restart the begin phase with a new fiber. + return remountFiber(current, workInProgress, clonedWorkInProgress); } } diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index 7c9df820b98c5..a3293218e9ea5 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -32,6 +32,10 @@ import type {UpdateQueue} from './ReactUpdateQueue.old'; import checkPropTypes from 'shared/checkPropTypes'; +import { + isDevToolsPresent, + onClonedForForceRemount, +} from './ReactFiberDevToolsHook.old'; import { IndeterminateComponent, FunctionComponent, @@ -3194,19 +3198,21 @@ function beginWork( if (__DEV__) { if (workInProgress._debugNeedsRemount && current !== null) { - // This will restart the begin phase with a new fiber. - return remountFiber( - current, - workInProgress, - createFiberFromTypeAndProps( - workInProgress.type, - workInProgress.key, - workInProgress.pendingProps, - workInProgress._debugOwner || null, - workInProgress.mode, - workInProgress.lanes, - ), + const clonedWorkInProgress = createFiberFromTypeAndProps( + workInProgress.type, + workInProgress.key, + workInProgress.pendingProps, + workInProgress._debugOwner || null, + workInProgress.mode, + workInProgress.lanes, ); + + if (isDevToolsPresent) { + onClonedForForceRemount(workInProgress, clonedWorkInProgress); + } + + // This will restart the begin phase with a new fiber. + return remountFiber(current, workInProgress, clonedWorkInProgress); } } diff --git a/packages/react-reconciler/src/ReactFiberDevToolsHook.new.js b/packages/react-reconciler/src/ReactFiberDevToolsHook.new.js index e7bde59e20cba..9d2f9243a2752 100644 --- a/packages/react-reconciler/src/ReactFiberDevToolsHook.new.js +++ b/packages/react-reconciler/src/ReactFiberDevToolsHook.new.js @@ -166,3 +166,28 @@ export function onCommitUnmount(fiber: Fiber) { } } } + +export function onClonedForForceRemount( + oldWorkInProgress: Fiber, + newWorkInProgress: Fiber, +) { + if ( + injectedHook && + typeof injectedHook.onClonedForForceRemount === 'function' + ) { + try { + injectedHook.onClonedForForceRemount( + rendererID, + oldWorkInProgress, + newWorkInProgress, + ); + } catch (err) { + if (__DEV__) { + if (!hasLoggedError) { + hasLoggedError = true; + console.error('React instrumentation encountered an error: %s', err); + } + } + } + } +} diff --git a/packages/react-reconciler/src/ReactFiberDevToolsHook.old.js b/packages/react-reconciler/src/ReactFiberDevToolsHook.old.js index 494138685e104..198e6233c0b9d 100644 --- a/packages/react-reconciler/src/ReactFiberDevToolsHook.old.js +++ b/packages/react-reconciler/src/ReactFiberDevToolsHook.old.js @@ -166,3 +166,28 @@ export function onCommitUnmount(fiber: Fiber) { } } } + +export function onClonedForForceRemount( + oldWorkInProgress: Fiber, + newWorkInProgress: Fiber, +) { + if ( + injectedHook && + typeof injectedHook.onClonedForForceRemount === 'function' + ) { + try { + injectedHook.onClonedForForceRemount( + rendererID, + oldWorkInProgress, + newWorkInProgress, + ); + } catch (err) { + if (__DEV__) { + if (!hasLoggedError) { + hasLoggedError = true; + console.error('React instrumentation encountered an error: %s', err); + } + } + } + } +} diff --git a/packages/react-reconciler/src/ReactFiberHotReloading.new.js b/packages/react-reconciler/src/ReactFiberHotReloading.new.js index 4c9eaf010125c..61ce24224c3e3 100644 --- a/packages/react-reconciler/src/ReactFiberHotReloading.new.js +++ b/packages/react-reconciler/src/ReactFiberHotReloading.new.js @@ -318,6 +318,7 @@ function scheduleFibersWithFamiliesRecursively( if (needsRemount) { fiber._debugNeedsRemount = true; } + if (needsRemount || needsRender) { scheduleUpdateOnFiber(fiber, SyncLane, NoTimestamp); } diff --git a/packages/react-reconciler/src/ReactFiberHotReloading.old.js b/packages/react-reconciler/src/ReactFiberHotReloading.old.js index ee0616fae79c0..475b449541d16 100644 --- a/packages/react-reconciler/src/ReactFiberHotReloading.old.js +++ b/packages/react-reconciler/src/ReactFiberHotReloading.old.js @@ -318,6 +318,7 @@ function scheduleFibersWithFamiliesRecursively( if (needsRemount) { fiber._debugNeedsRemount = true; } + if (needsRemount || needsRender) { scheduleUpdateOnFiber(fiber, SyncLane, NoTimestamp); }