Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear named hooks Suspense and AST cache after a Fast Refresh #21891

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('parseHookNames', () => {

inspectHooks = require('react-debug-tools/src/ReactDebugHooks')
.inspectHooks;
parseHookNames = require('../parseHookNames').default;
parseHookNames = require('../parseHookNames').parseHookNames;

// Jest (jest-runner?) configures Errors to automatically account for source maps.
// This changes behavior between our tests and the browser.
Expand Down Expand Up @@ -158,6 +158,10 @@ describe('parseHookNames', () => {
]);
});

// TODO Test that cache purge works

// TODO Test that cached metadata is purged when Fast Refresh scheduled

describe('inline, external and bundle source maps', () => {
it('should work for simple components', async () => {
async function test(path, name = 'Component') {
Expand Down
5 changes: 3 additions & 2 deletions packages/react-devtools-extensions/src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
getSavedComponentFilters,
getShowInlineWarningsAndErrors,
} from 'react-devtools-shared/src/utils';
import parseHookNames from './parseHookNames';
import {parseHookNames, purgeCachedMetadata} from './parseHookNames';
import {
localStorageGetItem,
localStorageRemoveItem,
Expand Down Expand Up @@ -215,9 +215,10 @@ function createPanelIfReactLoaded() {
browserTheme: getBrowserTheme(),
componentsPortalContainer,
enabledInspectedElementContextMenu: true,
loadHookNamesFunction: parseHookNames,
loadHookNames: parseHookNames,
overrideTab,
profilerPortalContainer,
purgeCachedHookNamesMetadata: purgeCachedMetadata,
showTabBar: false,
store,
warnIfUnsupportedVersionDetected: true,
Expand Down
7 changes: 6 additions & 1 deletion packages/react-devtools-extensions/src/parseHookNames.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const originalURLToMetadataCache: LRUCache<
},
});

export default async function parseHookNames(
export async function parseHookNames(
hooksTree: HooksTree,
): Thenable<HookNames | null> {
if (!enableHookNameParsing) {
Expand Down Expand Up @@ -623,3 +623,8 @@ function updateLruCache(
});
return Promise.resolve();
}

export function purgeCachedMetadata(): void {
originalURLToMetadataCache.reset();
runtimeURLToMetadataCache.reset();
}
8 changes: 8 additions & 0 deletions packages/react-devtools-shared/src/backend/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,14 @@ export default class Agent extends EventEmitter<{|
this.emit('traceUpdates', nodes);
};

onFastRefreshScheduled = () => {
if (__DEBUG__) {
debug('onFastRefreshScheduled');
}

this._bridge.send('fastRefreshScheduled');
};

onHookOperations = (operations: Array<number>) => {
if (__DEBUG__) {
debug(
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/backend/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export function initBackend(
agent.onUnsupportedRenderer(id);
}),

hook.sub('fastRefreshScheduled', agent.onFastRefreshScheduled),
hook.sub('operations', agent.onHookOperations),
hook.sub('traceUpdates', agent.onTraceUpdates),

Expand Down
17 changes: 17 additions & 0 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,7 @@ export function attach(
overrideProps,
overridePropsDeletePath,
overridePropsRenamePath,
scheduleRefresh,
setErrorHandler,
setSuspenseHandler,
scheduleUpdate,
Expand All @@ -579,6 +580,22 @@ export function attach(
typeof setSuspenseHandler === 'function' &&
typeof scheduleUpdate === 'function';

if (typeof scheduleRefresh === 'function') {
// When Fast Refresh updates a component, the frontend may need to purge cached information.
// For example, ASTs cached for the component (for named hooks) may no longer be valid.
// Send a signal to the frontend to purge this cached information.
// The "fastRefreshScheduled" dispatched is global (not Fiber or even Renderer specific).
// This is less effecient since it means the front-end will need to purge the entire cache,
// but this is probably an okay trade off in order to reduce coupling between the DevTools and Fast Refresh.
renderer.scheduleRefresh = (...args) => {
try {
hook.emit('fastRefreshScheduled');
} finally {
return scheduleRefresh(...args);
}
};
}
Comment on lines +583 to +597
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I'd like your input on, @gaearon.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍🏼


// Tracks Fibers with recently changed number of error/warning messages.
// These collections store the Fiber rather than the ID,
// in order to avoid generating an ID for Fibers that never get mounted
Expand Down
2 changes: 2 additions & 0 deletions packages/react-devtools-shared/src/backend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ export type ReactRenderer = {
Mount?: any,
// Only injected by React v17.0.3+ in DEV mode
setErrorHandler?: ?(shouldError: (fiber: Object) => ?boolean) => void,
// Intentionally opaque type to avoid coupling DevTools to different Fast Refresh versions.
scheduleRefresh?: Function,
...
};

Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ type UpdateConsolePatchSettingsParams = {|
export type BackendEvents = {|
bridgeProtocol: [BridgeProtocol],
extensionBackendInitialized: [],
fastRefreshScheduled: [],
inspectedElement: [InspectedElementPayload],
isBackendStorageAPISupported: [boolean],
isSynchronousXHRSupported: [boolean],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// @flow

import {createContext} from 'react';
import type {
LoadHookNamesFunction,
PurgeCachedHookNamesMetadata,
} from '../DevTools';

export type Context = {
loadHookNames: LoadHookNamesFunction | null,
purgeCachedMetadata: PurgeCachedHookNamesMetadata | null,
};

const HookNamesContext = createContext<Context>({
loadHookNames: null,
purgeCachedMetadata: null,
});
HookNamesContext.displayName = 'HookNamesContext';

export default HookNamesContext;
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ import {
inspectElement,
} from 'react-devtools-shared/src/inspectedElementCache';
import {
clearHookNamesCache,
hasAlreadyLoadedHookNames,
loadHookNames,
} from 'react-devtools-shared/src/hookNamesCache';
import LoadHookNamesFunctionContext from 'react-devtools-shared/src/devtools/views/Components/LoadHookNamesFunctionContext';
import HookNamesContext from 'react-devtools-shared/src/devtools/views/Components/HookNamesContext';
import {SettingsContext} from '../Settings/SettingsContext';

import type {HookNames} from 'react-devtools-shared/src/types';
Expand Down Expand Up @@ -63,7 +64,10 @@ export type Props = {|

export function InspectedElementContextController({children}: Props) {
const {selectedElementID} = useContext(TreeStateContext);
const loadHookNamesFunction = useContext(LoadHookNamesFunctionContext);
const {
loadHookNames: loadHookNamesFunction,
purgeCachedMetadata,
} = useContext(HookNamesContext);
const bridge = useContext(BridgeContext);
const store = useContext(StoreContext);
const {parseHookNames: parseHookNamesByDefault} = useContext(SettingsContext);
Expand Down Expand Up @@ -150,6 +154,24 @@ export function InspectedElementContextController({children}: Props) {
[setState, state],
);

useEffect(() => {
if (enableHookNameParsing) {
if (typeof purgeCachedMetadata === 'function') {
// When Fast Refresh updates a component, any cached AST metadata may be invalid.
const fastRefreshScheduled = () => {
startTransition(() => {
clearHookNamesCache();
purgeCachedMetadata();
refresh();
});
};
bridge.addListener('fastRefreshScheduled', fastRefreshScheduled);
return () =>
bridge.removeListener('fastRefreshScheduled', fastRefreshScheduled);
}
}
}, [bridge]);

// Reset path now that we've asked the backend to hydrate it.
// The backend is stateful, so we don't need to remember this path the next time we inspect.
useEffect(() => {
Expand Down

This file was deleted.

22 changes: 16 additions & 6 deletions packages/react-devtools-shared/src/devtools/views/DevTools.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import TabBar from './TabBar';
import {SettingsContextController} from './Settings/SettingsContext';
import {TreeContextController} from './Components/TreeContext';
import ViewElementSourceContext from './Components/ViewElementSourceContext';
import LoadHookNamesFunctionContext from './Components/LoadHookNamesFunctionContext';
import HookNamesContext from './Components/HookNamesContext';
import {ProfilerContextController} from './Profiler/ProfilerContext';
import {ModalDialogContextController} from './ModalDialog';
import ReactLogo from './ReactLogo';
Expand Down Expand Up @@ -51,6 +51,7 @@ export type ViewElementSource = (
export type LoadHookNamesFunction = (
hooksTree: HooksTree,
) => Thenable<HookNames>;
export type PurgeCachedHookNamesMetadata = () => void;
export type ViewAttributeSource = (
id: number,
path: Array<string | number>,
Expand Down Expand Up @@ -87,7 +88,8 @@ export type Props = {|
// Loads and parses source maps for function components
// and extracts hook "names" based on the variables the hook return values get assigned to.
// Not every DevTools build can load source maps, so this property is optional.
loadHookNamesFunction?: ?LoadHookNamesFunction,
loadHookNames?: ?LoadHookNamesFunction,
purgeCachedHookNamesMetadata?: ?PurgeCachedHookNamesMetadata,
|};

const componentsTab = {
Expand All @@ -112,9 +114,10 @@ export default function DevTools({
componentsPortalContainer,
defaultTab = 'components',
enabledInspectedElementContextMenu = false,
loadHookNamesFunction,
loadHookNames,
overrideTab,
profilerPortalContainer,
purgeCachedHookNamesMetadata,
showTabBar = false,
store,
warnIfLegacyBackendDetected = false,
Expand Down Expand Up @@ -149,6 +152,14 @@ export default function DevTools({
[enabledInspectedElementContextMenu, viewAttributeSourceFunction],
);

const hookNamesContext = useMemo(
() => ({
loadHookNames: loadHookNames || null,
purgeCachedMetadata: purgeCachedHookNamesMetadata || null,
}),
[loadHookNames, purgeCachedHookNamesMetadata],
);

const devToolsRef = useRef<HTMLElement | null>(null);

useEffect(() => {
Expand Down Expand Up @@ -204,8 +215,7 @@ export default function DevTools({
componentsPortalContainer={componentsPortalContainer}
profilerPortalContainer={profilerPortalContainer}>
<ViewElementSourceContext.Provider value={viewElementSource}>
<LoadHookNamesFunctionContext.Provider
value={loadHookNamesFunction || null}>
<HookNamesContext.Provider value={hookNamesContext}>
<TreeContextController>
<ProfilerContextController>
<div className={styles.DevTools} ref={devToolsRef}>
Expand Down Expand Up @@ -240,7 +250,7 @@ export default function DevTools({
</div>
</ProfilerContextController>
</TreeContextController>
</LoadHookNamesFunctionContext.Provider>
</HookNamesContext.Provider>
</ViewElementSourceContext.Provider>
</SettingsContextController>
<UnsupportedBridgeProtocolDialog />
Expand Down
6 changes: 5 additions & 1 deletion packages/react-devtools-shared/src/hookNamesCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function readRecord<T>(record: Record<T>): ResolvedRecord<T> | RejectedRecord {
// Otherwise, refreshing the inspected element cache would also clear this cache.
// TODO Rethink this if the React API constraints change.
// See https://github.com/reactwg/react-18/discussions/25#discussioncomment-980435
const map: WeakMap<Element, Record<HookNames>> = new WeakMap();
let map: WeakMap<Element, Record<HookNames>> = new WeakMap();

export function hasAlreadyLoadedHookNames(element: Element): boolean {
const record = map.get(element);
Expand Down Expand Up @@ -181,3 +181,7 @@ export function getHookSourceLocationKey({
}
return `${fileName}:${lineNumber}:${columnNumber}`;
}

export function clearHookNamesCache(): void {
map = new WeakMap();
}
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,5 +87,6 @@ export type HookNames = Map<HookSourceLocationKey, HookName>;
export type LRUCache<K, V> = {|
get: (key: K) => V,
has: (key: K) => boolean,
reset: () => void,
set: (key: K, value: V) => void,
|};