Repository navigation
lib: add FileAPI support - #67277
lib: add FileAPI support#67277avivkeller wants to merge 4 commits into
Conversation
Signed-off-by: avivkeller <me@aviv.sh>
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67277 +/- ##
=========================================
Coverage 90.68% 90.68%
=========================================
Files 797 799 +2
Lines 279367 280530 +1163
Branches 53737 53882 +145
=========================================
+ Hits 253344 254403 +1059
- Misses 16319 16414 +95
- Partials 9704 9713 +9
🚀 New features to boost your workflow:
|
Signed-off-by: avivkeller <me@aviv.sh>
jasnell
left a comment
There was a problem hiding this comment.
Please mark initially as experimental.
| A browser-compatible implementation of {FileReaderSync} from the [File API][]. | ||
| It is exposed only inside a [Web Worker][] (`--experimental-web-worker`), | ||
| matching `[Exposed=(DedicatedWorker,SharedWorker)]`. It is not available on | ||
| the main thread or in threads created with `node:worker_threads`. |
There was a problem hiding this comment.
I question whether this restriction is actually necessary in Node.js. Specifically the "It is not available on the main thread" or in worker threads.
The restrictions that apply to the browser do not necessarily apply here.
There was a problem hiding this comment.
This was marked resolved but it's still limited to web workers. The question is still open here.
|
A separate AI-agent review flagged the following additional items:
|
Signed-off-by: avivkeller <me@aviv.sh>
| added: REPLACEME | ||
| --> | ||
|
|
||
| > Stability: 1 - Experimental |
panva
left a comment
There was a problem hiding this comment.
Shouldn't this now be semver-minor with the new globals behind a default-off experimental flag?
The empty-MIME data URL results follow the current package-data wording, while WPT expects application/octet-stream. Progress counter initialization is also underspecified; that leaves an interoperability decision rather than a requirement to always report zero.
| // | ||
| // "Event handler for the load or error events could have started another | ||
| // load, if that happens the loadend event for this load is not fired." | ||
| queueFileReadingTask(fr, generation, () => { |
There was a problem hiding this comment.
The read operation fires loadend in the same task as load/error. Scheduling another task here lets a setImmediate scheduled inside onload start a second read and suppress the first read's loadend.
Please perform the state check and dispatch within the completion task.
const reader = new FileReader();
reader.onload = () => setImmediate(() => console.log('immediate'));
reader.onloadend = () => console.log('loadend');
reader.readAsText(new Blob(['x']));Prints immediate then loadend; expected loadend then immediate.
| // "Let promise be the result of reading all bytes from stream with | ||
| // reader." | ||
| // "Wait for promise to be fulfilled or rejected." | ||
| const buffer = getBlobDataSync(blob); |
There was a problem hiding this comment.
undefined here means this helper cannot obtain the bytes synchronously, not that the Blob is unreadable. All four methods consequently throw NotReadableError for a healthy fs.openAsBlob() result that FileReader reads successfully.
The synchronous methods must wait for the read and return its bytes. Can we support file-backed Blobs through a synchronous read path?
import { openAsBlob } from 'node:fs';
import { isMainThread } from 'node:worker_threads';
if (isMainThread) {
const worker = new Worker(import.meta.url, { type: 'module' });
worker.onmessage = ({ data }) => console.log(data);
} else {
const blob = await openAsBlob(import.meta.filename);
try {
postMessage(new FileReaderSync().readAsText(blob));
} catch (err) {
postMessage(err.name);
}
close();
}Prints NotReadableError; expected the source file contents.
| // "5. Let stream be the result of calling get stream on blob." | ||
| let stream; | ||
| try { | ||
| stream = createBlobReaderStream(blob[kHandle].getReader()); |
There was a problem hiding this comment.
This helper collapses native file failures into NotReadableError. fs.openAsBlob(path), followed by deleting the file and calling readAsText(blob), reproduces that error.
File API §7.1 requires NotFoundError when the resource cannot be found. Please preserve the underlying failure reason through the Blob plumbing so this API can expose the required exception.
import { openAsBlob, unlinkSync, writeFileSync } from 'node:fs';
const path = new URL(`./filereader-repro-${process.pid}.txt`, import.meta.url);
writeFileSync(path, 'x');
const blob = await openAsBlob(path);
unlinkSync(path);
const reader = new FileReader();
reader.onerror = () => console.log(reader.error.name);
reader.readAsText(blob);Prints NotReadableError; expected NotFoundError.
| exposeInterface(globalThis, 'WorkerNavigator', WorkerNavigator); | ||
| exposeInterface(globalThis, 'WorkerLocation', WorkerLocation); | ||
| // [Exposed=(DedicatedWorker,SharedWorker)] | ||
| exposeLazyInterfaces(globalThis, 'internal/filereader', ['FileReaderSync']); |
There was a problem hiding this comment.
Please gate this on --experimental-fileapi. Worker preparation removes the File API globals when the flag is disabled, but this runs afterward and unconditionally reinstalls FileReaderSync. A Web Worker started with only --experimental-web-worker therefore still gets the experimental interface.
const worker = new Worker(
'data:text/javascript,postMessage(typeof FileReaderSync);close()',
);
worker.onmessage = ({ data }) => console.log(data);Prints function; expected undefined without --experimental-fileapi.
| constructor(type, eventInitDict = kEmptyObject) { | ||
| const prefix = "Failed to construct 'ProgressEvent'"; | ||
| requiredArguments(arguments.length, 1, { prefix }); | ||
| const trusted = eventInitDict != null && |
There was a problem hiding this comment.
This reads argument 2 before converting argument 1, contrary to Web IDL's conversion order. With Symbol() as type and a Proxy dictionary whose get trap throws a sentinel, construction throws the sentinel instead of the required TypeError.
Please convert type first and keep internal trust handling outside the public dictionary conversion.
new ProgressEvent(Symbol(), new Proxy({}, {
get() { throw new Error('argument 2 was read'); },
}));Throws Error: argument 2 was read; expected TypeError before the dictionary is accessed.
| if (mimeType === '') { | ||
| return `data:;base64,${encoded}`; | ||
| } | ||
| return `data:${mimeType};base64,${encoded}`; |
There was a problem hiding this comment.
Data URL serialization needs another look for MIME types containing URL delimiters. text/plain;x=# currently produces an unfetchable URL. RFC 2397 §3 calls for escaping where necessary, although the exact File API serialization algorithm remains unresolved.
const reader = new FileReader();
reader.onload = () => fetch(reader.result).catch(console.log);
reader.readAsDataURL(new Blob(['hello'], { type: 'text/plain;x=#' }));This logs TypeError: fetch failed; the generated URL cannot be fetched back to the original bytes.
Adds support for the following FileAPI interfaces:
FileReaderFileReaderSyncProgressEvent