feat: implement FileReader by KhafraDev · Pull Request #1690 · nodejs/undici

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

❌ Patch coverage is 11.11111% with 264 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.55%. Comparing base (cc58f6e) to head (824c0bb).
⚠️ Report is 1818 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/fileapi/util.js	8.62%	106 Missing ⚠️
lib/fileapi/filereader.js	9.00%	101 Missing ⚠️
lib/fileapi/encoding.js	2.32%	42 Missing ⚠️
lib/fileapi/progressevent.js	23.52%	13 Missing ⚠️
lib/fetch/webidl.js	33.33%	2 Missing ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina

lgtm

ronag

Amazing work!

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

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

      "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

Comment thread index.js

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

Uh oh!

Conversation

KhafraDev commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Oct 8, 2022

Uh oh!

KhafraDev commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmywarting commented Oct 8, 2022

Uh oh!

jimmywarting commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KhafraDev commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Oct 17, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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