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

Adds Readable stream support when saving #67

Merged
merged 17 commits into from
Nov 29, 2021
Merged

Conversation

seanaye
Copy link
Contributor

@seanaye seanaye commented Sep 7, 2021

This PR allows passing a readable stream to the fileSave function instead of a blob. When supported this will pipe bytes directly into a writable stream, which reduces memory usage on large files (prevents copying). When not supported, the stream will simply be converted to a blob

closes #66

@google-cla
Copy link

google-cla bot commented Sep 7, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Member

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is headed in a great direction. Just one remark regarding the wrapping object. (Ignore the other comment that I have clarified myself.)

index.d.ts Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
Copy link
Member

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comment suggestions.

Update comment

Co-authored-by: Thomas Steiner <tomac@google.com>
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Update comment clarity

Co-authored-by: Thomas Steiner <tomac@google.com>
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

seanaye and others added 2 commits September 8, 2021 10:40
Update comment

Co-authored-by: Thomas Steiner <tomac@google.com>
Update comment

Co-authored-by: Thomas Steiner <tomac@google.com>
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Remove comment

Co-authored-by: Thomas Steiner <tomac@google.com>
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Remove comment

Co-authored-by: Thomas Steiner <tomac@google.com>
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

seanaye and others added 2 commits September 8, 2021 10:41
Update comment

Co-authored-by: Thomas Steiner <tomac@google.com>
Update comment

Co-authored-by: Thomas Steiner <tomac@google.com>
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Sep 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@seanaye
Copy link
Contributor Author

seanaye commented Sep 8, 2021

@googlebot I signed it!

index.d.ts Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/fs-access/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
src/legacy/file-save.mjs Outdated Show resolved Hide resolved
Copy link
Member

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, here's the way this could work:

You pass it either a Blob, or a Response, which can contain a ReadableStream and has a type attribute:

const response = new Response(new ReadableStream(), {
  headers: {
    "content-type": "text/plain",
  },
});

This means we don't need the weird object! I have made suggestions to implement this below, but I probably missed stuff (did it on the front-end without actually testing), so please carefully check this again.

@tomayac
Copy link
Member

tomayac commented Sep 20, 2021

@seanaye Are you OK with the solution I sketched in #67 (review)?

@seanaye
Copy link
Contributor Author

seanaye commented Sep 20, 2021

Yea definitely, sorry haven't had the time to make the changes

Copy link
Member

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tomayac tomayac merged commit 6499b04 into GoogleChromeLabs:main Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Streams API Support
2 participants