Skip to content

feat(streams): detach buffer using structuredClone#36723

Open
marcosc90 wants to merge 1 commit intoweb-platform-tests:masterfrom
marcosc90:use-structured-clone
Open

feat(streams): detach buffer using structuredClone#36723
marcosc90 wants to merge 1 commit intoweb-platform-tests:masterfrom
marcosc90:use-structured-clone

Conversation

@marcosc90
Copy link
Copy Markdown
Contributor

This PR adds structuredClone as a fallback to detach an ArrayBuffer if postMessage is not defined, so the test can be used in Deno

if(typeof postMessage === 'function') {
postMessage(buffer, '*', [buffer]);
} else {
structuredClone(buffer, { transfer: [buffer] });
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Left postMessage in case there's a platform running this test that doesn't yet support structuredClone

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we have a separate test with structuredClone below, and add an assert_implements_optional here so that it can pass in Deno? With the current PR Deno and browsers will have different code paths which wouldn't be ideal.

@wanderview wanderview removed their assignment Oct 31, 2022
Copy link
Copy Markdown
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

Cool. I wonder if it is worth renaming the file from .window.js to .any.js. This would improve coverage in many browsers, but not all supported structuredClone in workers according to mdn: https://developer.mozilla.org/en-US/docs/Web/API/structuredClone

Copy link
Copy Markdown
Member

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

(As commented)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants