Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: FileReader API #6673

Merged
merged 18 commits into from Aug 11, 2020
Merged

feat: FileReader API #6673

merged 18 commits into from Aug 11, 2020

Conversation

@PaulThompson
Copy link
Contributor

PaulThompson commented Jul 7, 2020

Implementation of all of FileReader

Addresses: #5249
Related to: #4142
#4581

Support the old but de-facto standard FileReader API.
As used by the browser-flavoured aws-sdk

Interface based on typescript lib.dom.ts FileReader interface.

FileReader js implementation from notes on https://w3c.github.io/FileAPI/
and based on body of deno blob.ts readBytes function.

PR also adds ProgressEvent

@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch 2 times, most recently from f777eec to 9a0c928 Jul 9, 2020
@bartlomieju bartlomieju added this to the 1.3.0 milestone Jul 14, 2020
@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch from 9a0c928 to 00f448b Jul 25, 2020
@PaulThompson PaulThompson marked this pull request as ready for review Jul 25, 2020
@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch from 1c4b849 to ad19fa0 Jul 25, 2020
@ry
Copy link
Member

ry commented Jul 25, 2020

Wow I totally missed this PR. Are you able to pass the tests locally? We need to get this green in order to land.

cli/rt/21_filereader.js Show resolved Hide resolved
cli/rt/21_filereader.js Show resolved Hide resolved
@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch from fc3a5ef to bbc6112 Jul 26, 2020
@PaulThompson
Copy link
Contributor Author

PaulThompson commented Jul 26, 2020

Updated to use __bootstrap to obtain setTimeout and ProgressEvent functions.

I kept ProgressEvent .js implementation in its own file because it is a separate exposed constructor function on window than FileReader and is present (as an interface at least) in multiple places in dom and webworker .d.ts files already (https://deno.land/typedoc/interfaces/_deno_.progressevent.html)
It would be necessary to emit ProgressEvent to implement XMLHttpRequest

@CLAassistant
Copy link

CLAassistant commented Jul 27, 2020

CLA assistant check
All committers have signed the CLA.

@PaulThompson PaulThompson requested a review from bartlomieju Jul 28, 2020
@bartlomieju
Copy link
Contributor

bartlomieju commented Jul 28, 2020

Updated to use __bootstrap to obtain setTimeout and ProgressEvent functions.

I kept ProgressEvent .js implementation in its own file because it is a separate exposed constructor function on window than FileReader and is present (as an interface at least) in multiple places in dom and webworker .d.ts files already (https://deno.land/typedoc/interfaces/_deno_.progressevent.html)
It would be necessary to emit ProgressEvent to implement XMLHttpRequest

Okay, but it's not used in runtime anywhere besides 21_filereader.js. If we add more APIs using ProgressEvent then I'll be happy to move it into its own file. I'm not sure why there's ProgressEvent in typedoc but it's definitely outdated, it's not listed in https://doc.deno.land/https/github.com/denoland/deno/releases/latest/download/lib.deno.d.ts#Event

@bartlomieju
Copy link
Contributor

bartlomieju commented Jul 28, 2020

@PaulThompson is there anything preventing from implementing FileReader.abort()? We can of course do that in a separate PR, just curious on the reason.

@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch from bbc6112 to 2f641bc Jul 31, 2020
@PaulThompson
Copy link
Contributor Author

PaulThompson commented Jul 31, 2020

Moved ProgressEvent into 21_filereader.js
Kept window.__bootstrap.progressEvent separate from window.__bootstrap.fileReader (Don't mean to split hairs - not sure exactly what the preferred semantics are).

@PaulThompson
Copy link
Contributor Author

PaulThompson commented Jul 31, 2020

Added abort() function implementation & test.

@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch 2 times, most recently from 9a0c928 to b6dd32f Aug 2, 2020
@PaulThompson
Copy link
Contributor Author

PaulThompson commented Aug 2, 2020

@bartlomieju All changes done, checks green & rebased on master.

@bartlomieju
Copy link
Contributor

bartlomieju commented Aug 3, 2020

@PaulThompson thanks, I'll merge this PR next week in preparation for v1.3.0

@PaulThompson
Copy link
Contributor Author

PaulThompson commented Aug 9, 2020

Falling behind updates to master...
Failed tests on re-merge of master into the branch.

FileReader typescript interface taken from typescript lib.dom.ts
https://github.com/microsoft/TypeScript/blob/9a5c0074aa64f2a85b425b0e5e6d67c473113693/lib/lib.dom.d.ts#L5502

FileReader js implementation from notes on https://w3c.github.io/FileAPI/
and based on body of deno blob.ts readBytes function.
@PaulThompson PaulThompson force-pushed the PaulThompson:feat-filereader branch from 9d419f4 to 746a76a Aug 10, 2020
@PaulThompson
Copy link
Contributor Author

PaulThompson commented Aug 10, 2020

The branch's ci breaks from inclusion of "Op crate for Web APIs" #6906

@ry @bartlomieju

@bartlomieju
Copy link
Contributor

bartlomieju commented Aug 10, 2020

@PaulThompson thanks for the heads up, I'll be reviewing your PR this week in preparation for 1.3.0 and I'll fix it before landing.

Copy link
Contributor

bartlomieju left a comment

LGTM, thank you @PaulThompson and sorry for slow review

@bartlomieju bartlomieju merged commit eed77aa into denoland:master Aug 11, 2020
7 checks passed
7 checks passed
test_release macos-10.15
Details
test_release windows-2019
Details
test_release ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.