feat: implement FileReader #1690

KhafraDev · 2022-10-07T23:57:14Z

Benefits:

Allowed me to enable the idlharness tests for the FileAPI section (Blob, URL, File, etc.) - found 2 issues in File, 6 in Blob, and 2 in URL. The alternative was going through and disabling over 100 tests.
Deno and every browser implement this. The same arguments for adding btoa and atob could be made here as well.

The API, although outdated, is still perfectly usable in modern environments. Unlike XMLHttpRequest, does not come with any foot guns (synchronous requests).

codecov-commenter · 2022-10-08T00:01:19Z

Codecov Report

Base: 94.25% // Head: 89.55% // Decreases project coverage by -4.70% ⚠️

Coverage data is based on head (824c0bb) compared to base (cc58f6e).
Patch coverage: 11.11% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1690      +/-   ##
==========================================
- Coverage   94.25%   89.55%   -4.71%     
==========================================
  Files          53       58       +5     
  Lines        4965     5257     +292     
==========================================
+ Hits         4680     4708      +28     
- Misses        285      549     +264

Impacted Files	Coverage Δ
lib/fileapi/encoding.js	`2.32% <2.32%> (ø)`
lib/fileapi/util.js	`8.62% <8.62%> (ø)`
lib/fileapi/filereader.js	`9.00% <9.00%> (ø)`
lib/fileapi/progressevent.js	`23.52% <23.52%> (ø)`
lib/fetch/webidl.js	`93.65% <33.33%> (-0.98%)`	⬇️
index-fetch.js	`100.00% <100.00%> (ø)`
index.js	`100.00% <100.00%> (ø)`
lib/fetch/file.js	`91.00% <100.00%> (+0.18%)`	⬆️
lib/fetch/index.js	`83.36% <100.00%> (+0.03%)`	⬆️
lib/fileapi/symbols.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mcollina

lgtm

ronag

Amazing work!

@KhafraDev I see you don't have any public email address. Would you mind sending me an email?

index.js

targos

Please also update index-fetch.js

targos · 2022-10-08T07:19:54Z

Not objecting to it landing here, but I feel like it would be nice to have it in Node.js core independently of undici

KhafraDev · 2022-10-08T15:43:23Z

It would require a lot of work to port it to node core because it uses parts from the webidl and mime specs. Undici also implements File while node implements URL and Blob from the FileAPI spec.

~~I'm also not sure if node's WPT runner can handle idlharness tests as it currently does not do so.~~ edit: the WPTs just seem to be outdated for the FileAPI.

jimmywarting · 2022-10-08T18:02:17Z

think PR should rather be made to WPT to change the test to using the blobs new read methods instead...
The FileReader should be a own isolated independent test.

jimmywarting · 2022-10-08T18:39:31Z

The Blob class in NodeJS core is kind of B imo, specially when it comes to streaming the content of the blob. (see nodejs/node#42264) + it also lacks all those webidl stuff.

I think I also saw some changes/fixes you did to the Blob/File class that I think should rather be reflected onto the NodeJS Core Blob....

So I somewhat agree with @targos on this. either we have all the Blob/File stuff in NodeJS core, or everything is moved to from Core into Undici. But then we would have to move a bunch of things such as object URL and copy/postMessage stuff.

I also wish to see some form of "create a file backed up by the filesystem" to be implemented by NodeJS also. That would require the blob to keep track of certain file metadata. This is fetch-blob only main reason why it's still needed/around and why I haven't deprecated it yet and said: just use NodeJS built in stuff.

I also can't extend the NodeJS core Blob in fetch-blob b/c i want to create blobs backed up by the filesystem. wish is a bit sad b/c then my fetch-blob can't be transfered over with PostMessage + it's not an instance of NodeJS core Blob, and they can't work along with ObjectURLs or Worker 😞

As long as Blobs can only be constructed by some stuff in the memory, then they are pretty useless IMO. And you might just as well just use a ArrayBuffer/uint8array instead. Blob main purpose is kind of helping you to offload some of the data and have them being backed up by some file handle and offload some of the memory to a temporary location on the disc.

KhafraDev · 2022-10-08T19:13:39Z

it also lacks all those webidl stuff.

The problem with missing webidl is that it would be a pain to convert it to the node equivalent - it's simply easier to implement it in undici.

Regarding URL and Blob, I believe both of them interface with c++ at some point. It's not feasible to re-write both in js. Both File and FileReader are implemented in js (same with Deno).

If anything, File and FileReader could be implemented in node core directly eventually, however, considering that this FileReader implementation passes 95% of the WPTs already, it will incur very little maintenance burden (1 test with event ordering and 1 due to differences between a microtask and a html task queue).

resolved

mcollina · 2022-10-17T07:55:11Z

@KhafraDev this has quite a bit of conflicts.

mcollina

lgtm

SimenB · 2022-11-07T14:29:22Z

package.json

@@ -116,7 +116,8 @@
    "compilerOptions": {
      "esModuleInterop": true,
      "lib": [
-        "esnext"
+        "esnext",
+        "DOM"


this means the types of undici only works if dependent project also adds the dom types. That shouldn't be necessary for a module which explicitly doesn't work in the browser

Whether it works in the browser or not is not the relevant part. The types are and fetch is based on DOM types per the spec. So it's not weird that we also need to import DOM types as well...

this PR uses DOMException (and possibly more) from DOM types

Whether it works in the browser or not is not the relevant part.

It's relevant for consumers that undici doesn't work in a typescript environment that isn't configured to be run in the browser. But I can agree it's more of a symptom than a cause 🙂

unfortunately @types/node doesn't seem to include quite a bit. If you remove that line and run npm run test:typescript we get errors about missing DOMException and EventInit types.

DOMException doesn't exist in Node, so that makes sense. EventInit seems to be an interface? Not something in MDN at least

Ah. I tried to run typeof DOMException and got undefined, but I see I wrote typeof DOMEXception 🙈

Regardless, I'm sure definitelytyped would welcome a PR adding DOMException, EventInit and any other missing interfaces. But they shouldn't be pulled from dom.

EDIT: I do see EventInit there already: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/488ef79fe14ea591166813a01ba7f34dfd955b68/types/node/dom-events.d.ts#L78

EventInit doesn't seem like a problem regardless, we can create our own and it won't overlap with the existing type (although I'm not sure why I can't access it). If we could "feature detect" DOMException, and only use our own type when it doesn't exist, that would be good. That way, the types won't overlap when/if DOMException gets added to node types.

I tried adding /// <reference types="node/dom-events" /> but the type still couldn't be used. I'll play around a little when I get home.

I opened a PR to fix this #1762

I do think this is kind of a TypeScript issue: microsoft/TypeScript#43972

* feat: implement FileReader * fix: implement correct text decoding * fix: make readOperation sync * feat: implement abort * test: event order fails * fix: readAsArrayBuffer incorrect byteLength * fix: base64 data url & default mime type * test: add most remaining tests * fix: update index-fetch.js * fix: correct types

KhafraDev mentioned this pull request Oct 8, 2022

lib: make properties on Blob and URL enumerable nodejs/node#44918

Merged

mcollina approved these changes Oct 8, 2022

View reviewed changes

ronag reviewed Oct 8, 2022

View reviewed changes

index.js Show resolved Hide resolved

ronag approved these changes Oct 8, 2022

View reviewed changes

targos previously requested changes Oct 8, 2022

View reviewed changes

KhafraDev added 10 commits October 17, 2022 10:27

feat: implement FileReader

1c9fdcf

fix: implement correct text decoding

ffcc07d

fix: make readOperation sync

158ee3f

feat: implement abort

ed3ca9f

test: event order fails

c682efa

fix: readAsArrayBuffer incorrect byteLength

0125c0d

fix: base64 data url & default mime type

1ad2130

test: add most remaining tests

93bc8fb

fix: update index-fetch.js

7258f79

fix: correct types

824c0bb

KhafraDev force-pushed the filereader branch from 61a097a to 824c0bb Compare October 17, 2022 14:28

mcollina approved these changes Oct 18, 2022

View reviewed changes

mcollina merged commit dfee69f into nodejs:main Oct 18, 2022

KhafraDev deleted the filereader branch October 18, 2022 14:41

SimenB reviewed Nov 7, 2022

View reviewed changes

eXhumer mentioned this pull request Sep 1, 2024

Unable to access base Event properties from CloseEvent, ErrorEvent & MessageEvent for WebSocket #3524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement FileReader #1690

feat: implement FileReader #1690

KhafraDev commented Oct 7, 2022 •

edited

Loading

codecov-commenter commented Oct 8, 2022 •

edited

Loading

mcollina left a comment

ronag left a comment

targos left a comment

targos commented Oct 8, 2022

KhafraDev commented Oct 8, 2022 •

edited

Loading

jimmywarting commented Oct 8, 2022

jimmywarting commented Oct 8, 2022 •

edited

Loading

KhafraDev commented Oct 8, 2022 •

edited

Loading

mcollina commented Oct 17, 2022

mcollina left a comment

SimenB Nov 7, 2022

ronag Nov 7, 2022

SimenB Nov 7, 2022

KhafraDev Nov 7, 2022

SimenB Nov 7, 2022

SimenB Nov 7, 2022 •

edited

Loading

KhafraDev Nov 7, 2022

KhafraDev Nov 7, 2022

KhafraDev Nov 7, 2022

jimmywarting Nov 9, 2022

feat: implement FileReader #1690

feat: implement FileReader #1690

Conversation

KhafraDev commented Oct 7, 2022 • edited Loading

codecov-commenter commented Oct 8, 2022 • edited Loading

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

ronag left a comment

Choose a reason for hiding this comment

targos left a comment

Choose a reason for hiding this comment

targos commented Oct 8, 2022

KhafraDev commented Oct 8, 2022 • edited Loading

jimmywarting commented Oct 8, 2022

jimmywarting commented Oct 8, 2022 • edited Loading

KhafraDev commented Oct 8, 2022 • edited Loading

mcollina commented Oct 17, 2022

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KhafraDev commented Oct 7, 2022 •

edited

Loading

codecov-commenter commented Oct 8, 2022 •

edited

Loading

KhafraDev commented Oct 8, 2022 •

edited

Loading

jimmywarting commented Oct 8, 2022 •

edited

Loading

KhafraDev commented Oct 8, 2022 •

edited

Loading

SimenB Nov 7, 2022 •

edited

Loading