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: extract Blob's .arrayBuffer() & webidl changes #53372

Merged
merged 1 commit into from
Sep 26, 2024
Merged

buffer: extract Blob's .arrayBuffer() & webidl changes #53372

merged 1 commit into from
Sep 26, 2024

Conversation

KhafraDev
Copy link
Member

  • Extracts Blob.prototype.arrayBuffer so it cannot be overridden in .text(), etc.
  • Make .bytes() enumerable. I guess the WPT runner is not running the idlharness tests?
  • Make .text() return a Promise, rather than being explicitly async. This is a non-documented part of the webidl spec. Refs: lib: make fetch sync and return a Promise #49936

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 6, 2024
@KhafraDev KhafraDev force-pushed the blob-changes branch 2 times, most recently from ae277ce to b577692 Compare June 6, 2024 18:08
H4ad
H4ad reviewed Jun 9, 2024
View reviewed changes
lib/internal/blob.js Show resolved Hide resolved
test/parallel/test-blob.js Show resolved Hide resolved
@KhafraDev
buffer: extract Blob's .arrayBuffer() & webidl changes
9e435c2 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.
@codecov Codecov
Copy link

codecov bot commented Sep 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.25%. Comparing base (aac8ba7) to head (9e435c2).
Report is 9 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #53372      +/-   ##
==========================================
+ Coverage   88.24%   88.25%   +0.01%     
==========================================
  Files         651      651              
  Lines      183856   183863       +7     
  Branches    35853    35859       +6     
==========================================
+ Hits       162251   162276      +25     
+ Misses      14896    14891       -5     
+ Partials     6709     6696      -13
Files with missing lines Coverage Δ
lib/internal/blob.js 99.80% <100.00%> (+<0.01%) ⬆️

... and 38 files with indirect coverage changes

@KhafraDev
Copy link
Member Author

@nodejs/web-standards

Please review I am desperate.

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 26, 2024 •
edited by KhafraDev
Loading

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

@KhafraDev KhafraDev added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Sep 26, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 26, 2024
@nodejs-github-bot nodejs-github-bot merged commit da5887d into nodejs:main Sep 26, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in da5887d

targos pushed a commit that referenced this pull request Oct 4, 2024
@KhafraDev @targos
buffer: extract Blob's .arrayBuffer() & webidl changes
3a6e724 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: #49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.

PR-URL: #53372
Refs: #49936
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@KhafraDev @louwers
buffer: extract Blob's .arrayBuffer() & webidl changes
1218277 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.

PR-URL: nodejs#53372
Refs: nodejs#49936
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@H4ad H4ad H4ad 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KhafraDev @nodejs-github-bot @anonrig @H4ad