DevTools Suspense cache cleanup

[bvaughn]

Fixed a few things I noticed while testing #22263.

Commits can be reviewed independently.

[jstejada]

thanks! i've noticed that sometimes parsing hook names can fail silently, or just without any indication of failure in the UI or logs.. would this change address that issue?

[bvaughn]

thanks! i've noticed that sometimes parsing hook names can fail silently, or just without any indication of failure in the UI or logs.. would this change address that issue?

Yes, that's the goal. Should at least log an error now (to the nested devtools console)

[gaearon]

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.

[bvaughn]

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.

[bvaughn]

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?

[bvaughn]

Reverted in 3c3f072

[bvaughn]

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.

[jstejada]

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

[gaearon]

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.