fix(cli): don't store blob and data urls in the module cache

[nayeemrmn]

Fixes #18260.
Also follows on from #18253.

[aapoalas]

I'm not sure if there's a blob test for this already, but if not could you also test that importing the same blob URL in the same run twice gets the same module?

Something like this:

export let data = 3;
export const setData = d => { data = d; };

Create a blob out of that file, import it once, set data to 4, then import it a second time and verify that the data there is indeed 4.

I'm probably worrying for nothing, but seeing the cache drop out I thought that maybe, just maybe it would also mean that blob URL imports become unique instances.

[nayeemrmn]

I'm not sure if there's a blob test for this already, but if not could you also test that importing the same blob URL in the same run twice gets the same module?

Something like this:

export let data = 3;
export const setData = d => { data = d; };

maybe it would also mean that blob URL imports become unique instances.

@aapoalas I'm not sure what you're suggesting to test,

• To check that it's the same instance we could just do await import(blobUrl) == await import(blobUrl), but that's an engine/deno_core guarantee which doesn't query CLI's module loader.
• We could check that await import(blobUrl) and await import(blobUrl + "#1") give modules with the same behaviour... still seems esoteric. That implies the blob url might have been unbound after the first import for some reason (if anything that would occur right after creation leading to the first import failing).

Note that file URLs already opt out of FileFetcher's cache, it's a really high level one with like two beneath and one above it.

Actually, I've questioned why FileFetcher::cache is active for standard execution (e.g. run/test without --watch) given ProcState::graph_container. Pretty sure the only usage that really needs it is the LSP's ModuleRegistry. That would be a worthwhile investigation.

[aapoalas]

Yeah, I was precisely referring to a test like await import(blobUrl) === await import(blobUrl) but if we already have prior examples of skipping the cache then it sounds safe enough a change to me.

Looks good to me and nice work! Very nice diff :)

[kt3k]

Since this change, the islands in fresh don't build anymore, showing the below error (the error message is not that intelligible though). Maybe fresh was using these removed metadata?

✘ [ERROR] [unreachable] Not an ESM module. [plugin deno]

An error occurred during route handling or page rendering. Error: Build failed with 1 error:
error: [unreachable] Not an ESM module.
    at failureErrorWithLog (https://deno.land/x/[email protected]/mod.js:1612:15)
    at https://deno.land/x/[email protected]/mod.js:1024:25
    at runOnEndCallbacks (https://deno.land/x/[email protected]/mod.js:1447:45)
    at buildResponseToResult (https://deno.land/x/[email protected]/mod.js:1022:7)
    at https://deno.land/x/[email protected]/mod.js:1051:16
    at responseCallbacks.<computed> (https://deno.land/x/[email protected]/mod.js:673:9)
    at handleIncomingPacket (https://deno.land/x/[email protected]/mod.js:728:9)
    at readFromStdout (https://deno.land/x/[email protected]/mod.js:649:7)
    at https://deno.land/x/[email protected]/mod.js:1898:11