Skip to content

DevTools Suspense cache cleanup #22275

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.

Sign up for GitHub

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

Merged
bvaughn merged 3 commits into from
Sep 9, 2021
+120 −87
Merged

DevTools Suspense cache cleanup #22275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fcde7a
Hardened DevTools Suspense caches
Sep 8, 2021
c03fdbc
Guard against sourceMappingURL expression in code
Sep 8, 2021
28d5f06
Fixed a comment typo 🤡
Sep 8, 2021
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
77 changes: 42 additions & 35 deletions packages/react-devtools-shared/src/hookNamesCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,44 +110,51 @@ export function loadHookNames(

let didTimeout = false;

loadHookNamesFunction(hooksTree, fetchFileWithCaching).then(
function onSuccess(hookNames) {
if (didTimeout) {
return;
}

if (__DEBUG__) {
console.log('[hookNamesCache] onSuccess() hookNames:', hookNames);
}

if (hookNames) {
const resolvedRecord = ((newRecord: any): ResolvedRecord<HookNames>);
resolvedRecord.status = Resolved;
resolvedRecord.value = hookNames;
} else {
const notFoundRecord = ((newRecord: any): RejectedRecord);
notFoundRecord.status = Rejected;
notFoundRecord.value = null;
}

wake();
},
function onError(error) {
if (didTimeout) {
return;
}
const onSuccess = hookNames => {
if (didTimeout) {
return;
}

if (__DEBUG__) {
console.log('[hookNamesCache] onSuccess() hookNames:', hookNames);
}

if (__DEBUG__) {
console.log('[hookNamesCache] onError() error:', error);
}
if (hookNames) {
const resolvedRecord = ((newRecord: any): ResolvedRecord<HookNames>);
resolvedRecord.status = Resolved;
resolvedRecord.value = hookNames;
} else {
const notFoundRecord = ((newRecord: any): RejectedRecord);
notFoundRecord.status = Rejected;
notFoundRecord.value = null;
}

const thrownRecord = ((newRecord: any): RejectedRecord);
thrownRecord.status = Rejected;
thrownRecord.value = null;
wake();
};

wake();
},
);
const onError = error => {
if (didTimeout) {
return;
}

if (__DEBUG__) {
console.log('[hookNamesCache] onError()');
}

console.error(error);

const thrownRecord = ((newRecord: any): RejectedRecord);
thrownRecord.status = Rejected;
thrownRecord.value = null;

wake();
};

const thenable = loadHookNamesFunction(hooksTree, fetchFileWithCaching);
thenable.then(onSuccess, onError);
if (typeof (thenable: any).catch === 'function') {
Copy link
Collaborator

@gaearon gaearon Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This does not look right to me. Can you elaborate on the reasoning for why this makes sense?

Normally, catch is not a part of the Thenable contract altogether. In Promises, .catch(onError) is a convenience that translates to .then(undefined, onError). So from what I see in this implementation, if thenable is a Promise, the consequence should be that onError runs twice:

thenable.then(onSuccess, onError)
thenable.then(undefined, onError)

I don't think this is what was intended.

Do you have a specific example case that you're trying to fix here? I suspect there's some other problem that was covered up, and maybe this accidentally fixes it, but it seems like a wrong fix.

Copy link
Contributor Author

@bvaughn bvaughn Sep 9, 2021

Choose a reason for hiding this comment

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

As I understand it, catch should really only be necessary if an error was thrown inside of onSuccess or onError – but what I noticed yesterday when testing a source-map parsing problem (inside of the hooks/parseHookNames module) was that the onError callback wasn't being invoked, and the error was effectively being swallowed (until the timeout eventually fired).

I can try to repro this by backing out this change, if that would be helpful.

Copy link
Contributor Author

@bvaughn bvaughn Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I checked out bvaughn:devtools-dynamic-import-suspense-cache (#22263), and partially reverted this commit (24c2e27), and tried to repro the problem I saw yesterday and I'm not able. Kind of a head scratcher, but it tracks with my understanding of how .catch() works.

I can revert this specific change from this PR and leave the rest. Maybe @jstejada can weigh in if he sees another case where we aren't handling an error correctly in the cache?

Copy link
Contributor Author

@bvaughn bvaughn Sep 9, 2021

Choose a reason for hiding this comment

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

Reverted in 3c3f072

Copy link
Contributor Author

@bvaughn bvaughn Sep 9, 2021

Choose a reason for hiding this comment

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

Bit of follow-up.

After reverting that bit of the change, and using the test shell (with HMR on) for a little bit, I think I see the "uncaught error" issue I initially noticed. Digging in a little more, I think it actually comes from this code which doesn't pass the error handler:

react/packages/react-devtools-shared/src/inspectedElementCache.js

Lines 178 to 195 in 8f96c6b

inspectElementMutableSource({
bridge,
element,
path: null,
rendererID: ((rendererID: any): number),
}).then(
([
inspectedElement: InspectedElementFrontend,
responseType: InspectedElementResponseType,
]) => {
if (responseType === 'full-data') {
startTransition(() => {
const [key, value] = createCacheSeed(element, inspectedElement);
refresh(key, value);
});
}
},
);

So I think this was my mistake. Thanks for questioning this.

Copy link
Contributor

@jstejada jstejada Sep 9, 2021

Choose a reason for hiding this comment

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

not sure if related, but yes i am planning to look into a case where we seemed to not handle an error, will report back as to whether it's related to this or not

Copy link
Collaborator

@gaearon gaearon Sep 9, 2021

Choose a reason for hiding this comment

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

As I understand it, catch should really only be necessary if an error was thrown inside of onSuccess or onError

So I think it’s okay (and intentional) not to catch errors in these, just like React doesn’t. Because they should have the minimal logic that sets the values in the cache and have no reason to throw. If they do throw then that’s a bug in the cache, which should be surfaced as unhandled.

(thenable: any).catch(onError);
}

// Eventually timeout and stop trying to load names.
let timeoutID = setTimeout(function onTimeout() {
Expand Down
5 changes: 5 additions & 0 deletions .../react-devtools-shared/src/hooks/__tests__/__source__/ContainingStringSourceMappingURL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,17 @@ import React, {useState} from 'react';

// ?sourceMappingURL=([^\s'"]+)/gm

const abc = 'abc';
const string =
'sourceMappingURL=data:application/json;charset=utf-8;base64,' + abc;

export function Component() {
const [count, setCount] = useState(0);

return (
<div>
<p>You clicked {count} times</p>
<div>string: {string}</div>
<button onClick={() => setCount(count + 1)}>Click me</button>
</div>
);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/hooks/__tests__/parseHookNames-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe('parseHookNames', () => {
.Component;

fetchMock.mockIf(/.+$/, request => {
// Since th custom hook contains only unnamed hooks, the source code should not be loaded.
// Since the custom hook contains only unnamed hooks, the source code should not be loaded.
if (request.url.endsWith('useCustom.js')) {
throw Error(`Unexpected file request for "${request.url}"`);
}
Expand Down
83 changes: 49 additions & 34 deletions packages/react-devtools-shared/src/hooks/parseHookNames/loadSourceAndMetadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,14 @@ function extractAndLoadSourceMapJSON(
// Deduplicate fetches, since there can be multiple location keys per source map.
const dedupedFetchPromises = new Map();

if (__DEBUG__) {
console.log(
'extractAndLoadSourceMapJSON() load',
locationKeyToHookSourceAndMetadata.size,
'source maps',
);
}

const setterPromises = [];
locationKeyToHookSourceAndMetadata.forEach(hookSourceAndMetadata => {
const sourceMapRegex = / ?sourceMappingURL=([^\s'"]+)/gm;
Expand Down Expand Up @@ -177,45 +185,52 @@ function extractAndLoadSourceMapJSON(
const sourceMappingURL = sourceMappingURLMatch[1];
const hasInlineSourceMap = sourceMappingURL.indexOf('base64,') >= 0;
if (hasInlineSourceMap) {
// TODO (named hooks) deduplicate parsing in this branch (similar to fetching in the other branch)
// since there can be multiple location keys per source map.

// Web apps like Code Sandbox embed multiple inline source maps.
// In this case, we need to loop through and find the right one.
// We may also need to trim any part of this string that isn't based64 encoded data.
const trimmed = ((sourceMappingURL.match(
/base64,([a-zA-Z0-9+\/=]+)/,
): any): Array<string>)[1];
const decoded = withSyncPerformanceMark('decodeBase64String()', () =>
decodeBase64String(trimmed),
);

const sourceMapJSON = withSyncPerformanceMark(
'JSON.parse(decoded)',
() => JSON.parse(decoded),
);
try {
// TODO (named hooks) deduplicate parsing in this branch (similar to fetching in the other branch)
// since there can be multiple location keys per source map.

// Web apps like Code Sandbox embed multiple inline source maps.
// In this case, we need to loop through and find the right one.
// We may also need to trim any part of this string that isn't based64 encoded data.
const trimmed = ((sourceMappingURL.match(
/base64,([a-zA-Z0-9+\/=]+)/,
): any): Array<string>)[1];
const decoded = withSyncPerformanceMark(
'decodeBase64String()',
() => decodeBase64String(trimmed),
);

if (__DEBUG__) {
console.groupCollapsed(
'extractAndLoadSourceMapJSON() Inline source map',
const sourceMapJSON = withSyncPerformanceMark(
'JSON.parse(decoded)',
() => JSON.parse(decoded),
);
console.log(sourceMapJSON);
console.groupEnd();
}

// Hook source might be a URL like "https://4syus.csb.app/src/App.js"
// Parsed source map might be a partial path like "src/App.js"
if (sourceMapIncludesSource(sourceMapJSON, runtimeSourceURL)) {
hookSourceAndMetadata.sourceMapJSON = sourceMapJSON;
if (__DEBUG__) {
console.groupCollapsed(
'extractAndLoadSourceMapJSON() Inline source map',
);
console.log(sourceMapJSON);
console.groupEnd();
}

// Hook source might be a URL like "https://4syus.csb.app/src/App.js"
// Parsed source map might be a partial path like "src/App.js"
if (sourceMapIncludesSource(sourceMapJSON, runtimeSourceURL)) {
hookSourceAndMetadata.sourceMapJSON = sourceMapJSON;

// OPTIMIZATION If we've located a source map for this source,
// we'll use it to retrieve the original source (to extract hook names).
// We only fall back to parsing the full source code is when there's no source map.
// The source is (potentially) very large,
// So we can avoid the overhead of serializing it unnecessarily.
hookSourceAndMetadata.runtimeSourceCode = null;
// OPTIMIZATION If we've located a source map for this source,
// we'll use it to retrieve the original source (to extract hook names).
// We only fall back to parsing the full source code is when there's no source map.
// The source is (potentially) very large,
// So we can avoid the overhead of serializing it unnecessarily.
hookSourceAndMetadata.runtimeSourceCode = null;

break;
break;
}
} catch (error) {
// We've likely encountered a string in the source code that looks like a source map but isn't.
// Maybe the source code contains a "sourceMappingURL" comment or soething similar.
// In either case, let's skip this and keep looking.
}
} else {
externalSourceMapURLs.push(sourceMappingURL);
Expand Down
40 changes: 23 additions & 17 deletions packages/react-devtools-shared/src/inspectedElementCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,28 +116,34 @@ export function inspectElement(
return null;
}

inspectElementMutableSource({
const onSuccess = ([inspectedElement: InspectedElementFrontend]) => {
const resolvedRecord = ((newRecord: any): ResolvedRecord<InspectedElementFrontend>);
resolvedRecord.status = Resolved;
resolvedRecord.value = inspectedElement;
wake();
};

const onError = error => {
if (newRecord.status === Pending) {
const rejectedRecord = ((newRecord: any): RejectedRecord);
rejectedRecord.status = Rejected;
rejectedRecord.value = `Could not inspect element with id "${element.id}". Error thrown:\n${error.message}`;
wake();
}
};

const thenable = inspectElementMutableSource({
bridge,
element,
path,
rendererID: ((rendererID: any): number),
}).then(
([inspectedElement: InspectedElementFrontend]) => {
const resolvedRecord = ((newRecord: any): ResolvedRecord<InspectedElementFrontend>);
resolvedRecord.status = Resolved;
resolvedRecord.value = inspectedElement;
wake();
},
});
thenable.then(onSuccess, onError);

if (typeof (thenable: any).catch === 'function') {
(thenable: any).catch();
}

error => {
if (newRecord.status === Pending) {
const rejectedRecord = ((newRecord: any): RejectedRecord);
rejectedRecord.status = Rejected;
rejectedRecord.value = `Could not inspect element with id "${element.id}". Error thrown:\n${error.message}`;
wake();
}
},
);
map.set(element, record);
}

Expand Down