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.

Sign up for GitHub

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

Jump to bottom

buffer: add Blob.prototype.stream method and other cleanups #39693

Closed
wants to merge 4 commits into from
Closed

buffer: add Blob.prototype.stream method and other cleanups #39693

wants to merge 4 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 7, 2021

There are a couple of changes here:

  1. It cleans up the Blob implementation to better match the spec.
  2. It adds the Blob.prototype.stream() method to get a ReadableStream from a Blob
  3. It enables the relevant web platform tests for stream()
  4. It adds an experimental implementation of URL.createObjectURL(), URL.revokeObjectURL(), and buffer.resolveObjectURL(). The URL.createObjectURL() and URL.revokeObjectURL() are standard Web Platform API.

/cc @bmeck

@jasnell
buffer: add Blob.prototype.stream method and other cleanups
7eb128a 
Adds the `stream()` method to get a `ReadableStream` for the `Blob`.
Also makes some other improvements to get the implementation closer
to the API standard definition.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
test: enable blob.prototype.stream tests
51fcc3e 
Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 7, 2021
aduh95
aduh95 reviewed Aug 7, 2021
View reviewed changes
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
addaleax
addaleax requested changes Aug 7, 2021
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/node_blob.cc Outdated Show resolved Hide resolved
src/node_blob.cc Outdated Show resolved Hide resolved
src/node_blob.cc Outdated Show resolved Hide resolved
src/node_blob.cc Outdated Show resolved Hide resolved
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 7, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bmeck
Copy link
Member

bmeck commented Aug 7, 2021

I know this is incremental/experimental, but can we add ESM integration by adding integration with:

This also could be done as a follow on PR.

@jasnell
Copy link
Member Author

jasnell commented Aug 7, 2021

@bmeck:

This also could be done as a follow on PR.

Yeah, definitely want to see that work done but let's definitely separate it out.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 8, 2021 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/39513/ 💚

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 8, 2021
@jasnell jasnell force-pushed the blob2 branch 2 times, most recently from 3ad7f65 to 0ae84b1 Compare August 8, 2021 18:26
@jasnell jasnell mentioned this pull request Aug 8, 2021
Closed
aduh95
aduh95 reviewed Aug 8, 2021
View reviewed changes
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/blob.js Show resolved Hide resolved
lib/internal/url.js Outdated Show resolved Hide resolved
@targos
Copy link
Member

targos commented Aug 9, 2021

Fixes: #39537

jimmywarting
jimmywarting reviewed Aug 9, 2021
View reviewed changes
lib/internal/blob.js Outdated Show resolved Hide resolved
jimmywarting
jimmywarting reviewed Aug 9, 2021
View reviewed changes
lib/internal/blob.js Outdated Show resolved Hide resolved
@jasnell
url,buffer: implement URL.createObjectURL
0998e6e 
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell removed the needs-ci PRs that need a full CI run. label Aug 9, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 9, 2021 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/39524/ 💚

@jimmywarting jimmywarting mentioned this pull request Aug 10, 2021
Closed
5 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 11, 2021 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/39543/ 💚

@jasnell
Copy link
Member Author

jasnell commented Aug 12, 2021

Landed in 87d6fd7...31d1d0c

@jasnell jasnell closed this Aug 12, 2021
jasnell added a commit that referenced this pull request Aug 12, 2021
@jasnell
buffer: add Blob.prototype.stream method and other cleanups
0bb2605 
Adds the `stream()` method to get a `ReadableStream` for the `Blob`.
Also makes some other improvements to get the implementation closer
to the API standard definition.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
jasnell added a commit that referenced this pull request Aug 12, 2021
@jasnell
test: enable blob.prototype.stream tests
793c08b 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
jasnell added a commit that referenced this pull request Aug 12, 2021
@jasnell
url,buffer: implement URL.createObjectURL
31d1d0c 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
@jasnell @danielleadams
buffer: add Blob.prototype.stream method and other cleanups
091a579 
Adds the `stream()` method to get a `ReadableStream` for the `Blob`.
Also makes some other improvements to get the implementation closer
to the API standard definition.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
@jasnell @danielleadams
test: enable blob.prototype.stream tests
53cf53c 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
@jasnell @danielleadams
url,buffer: implement URL.createObjectURL
37dda19 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@danielleadams danielleadams mentioned this pull request Aug 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@jimmywarting jimmywarting jimmywarting left review comments

@bmeck bmeck bmeck approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

@Qard Qard Qard approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jasnell @nodejs-github-bot @bmeck @targos @Qard @addaleax @jimmywarting @benjamingr @aduh95