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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from
+56 −32

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
buffer: extract Blob's .arrayBuffer() & webidl changes
b577692 
- 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.
H4ad
H4ad reviewed Jun 9, 2024
View reviewed changes
lib/internal/blob.js
@@ -436,6 +414,7 @@ ObjectDefineProperties(Blob.prototype, {
stream: kEnumerableProperty,
text: kEnumerableProperty,
arrayBuffer: kEnumerableProperty,
bytes: kEnumerableProperty,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a separate commit marked as fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a webidl change

test/parallel/test-blob.js
Comment on lines +498 to +509
(async () => {
const blob = new Blob(['hello']);
const { arrayBuffer } = Blob.prototype;

Blob.prototype.arrayBuffer = common.mustNotCall();

try {
assert.strictEqual(await blob.text(), 'hello');
} finally {
Blob.prototype.arrayBuffer = arrayBuffer;
}
})().then(common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

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

Does it worth to explicitly add a test for this behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@H4ad H4ad H4ad left review comments

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@KhafraDev @H4ad @nodejs-github-bot