module: support import()-ing blob: urls

[avivkeller]

Fixes #47573

Notable Change

The import() function can now receive blob: protocol URLs as an input. These blobs must be one of the following types:

• text/javascript for ES modules
• application/json for JSON
• application/wasm for Wasm

Blob URLs are registered within the current thread. Loaders will not be able to handle resolve Blob's from the main thread.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @redyetidev.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[codecov]

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.61%. Comparing base (4f14eb1) to head (b57674e).
Report is 96 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/load.js	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54647      +/-   ##
==========================================
+ Coverage   87.34%   87.61%   +0.27%     
==========================================
  Files         649      650       +1     
  Lines      182562   182847     +285     
  Branches    35023    35388     +365     
==========================================
+ Hits       159451   160198     +747     
+ Misses      16396    15926     -470     
- Partials     6715     6723       +8     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/get_format.js	89.23% <100.00%> (+0.34%)	⬆️
lib/internal/modules/esm/load.js	93.10% <85.71%> (+1.69%)	⬆️

... and 91 files with indirect coverage changes

[aduh95]

Can you add a test with a loader please? We need to validate the loader thread can resolve the blob URL

[aduh95]

We don't need the fixtures, what I want to validate is whether it's possible to load a blob: URL generated on the main thread from the loader thread.

Suggested change

	`register(${JSON.stringify(fixtures.fileURL('es-module-loaders/hooks-blobify.mjs'))}, pathToFileURL("./"));`,
	'--import', 'data:load_blob',
	'--eval=""',
	`register("data:text/javascript,");`,
	'--input-type=module',
	'--eval',
	`${createURL}await import(await createURL('text/javascript', 'console.log("Loaded");'));`,

[avivkeller]

IIUC blobs are limited to the process that they created from, and so that won't work.

import { resolveObjectURL } from 'buffer';
import { fileURLToPath } from 'url';
import { Worker, isMainThread, parentPort, workerData } from 'worker_threads';

if (isMainThread) {
    const value = await URL.createObjectURL(new Blob(['Data']));
    const worker = new Worker(fileURLToPath(import.meta.url), {
        workerData: { value }
    });

    console.log("MAIN", value, resolveObjectURL(value));
} else {
    const { value } = workerData;
    console.log("WORKER", value, resolveObjectURL(value));
}

I should however document that behavior.

[avivkeller]

It has documented, but if you aren't satisfied, LMK

[aduh95]

That's of course very dissatisfying if blob URLs are completely broken as soon as a hook is registered, IMO we shouldn't ship something that's going to break like that.

blobs are limited to the process that they created from

To be clear, it is the same process.

[avivkeller]

If I'm understanding how hooks work (like how I had this setup before), blob URLs will work, they just won't work from the CLI (parent->child process or parent->worker) Am I missing something in terms of how to test this?

Do you mean to call the hook with the blob URL, and load it there?

Sorry if I'm being confusing

[aduh95]

My suggestion is not to pass them via the CLI, just to have a test that validates blob URLs still work when there are loaders registered

[avivkeller]

After looking into it, this is not possible, so the following note has been added to the docs: "Blob URLs are registered within the current thread. Loaders will not be able to handle resolve Blob's from the main thread."

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61735/

[avivkeller]

CI: https://ci.nodejs.org/job/node-test-pull-request/61735/

CI is failing because the test isn't skipped when crypto support isn't provided. I'll fix that, along with adding the loaders test that @aduh95 suggested.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61746/

[avivkeller]

Several CI failures, however (AFAICT) none of them are related, could someone re-run the failed CIs?

[aduh95]

IMO it's not acceptable to land as stable a feature that can broken by another unflagged feature (cf #54647 (comment)). To resolve my objection, this would need to either fix the support if possible, or alternatively mark the feature as experimental, possibly behind a flag.

[avivkeller]

To resolve my objection, this would need to either fix the support if possible, or alternatively mark the feature as experimental, possibly behind a flag.

I'm happy to mark it experimental, as it also uses experimental APIs under the hood. IIUC the creation / resolution of blob urls is also experimental.

[avivkeller]

Re-requesting review now that this is marked experimental.

[mcollina]

good point @aduh95!

[avivkeller]

@mcollina are you also against this landing as experimental?

[mcollina]

I'm ok in adding as experimental if there is a path that loader hooks can be supported. It doesn't look the case atm.

[avivkeller]

Until Blobs can be supported in workers, I'm afraid it seems this is a no-go.