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

Add readable byte streams #1362

Merged
merged 13 commits into from
Aug 4, 2022

Conversation

MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens commented Jul 19, 2022

With Firefox 102 adding support for readable byte streams, we now have two browser engines (Chromium and Gecko) shipping this feature. This means we can finally add official type definitions! 😁

Most of work was already done in #890 but then those types had to be removed. This PR brings them back, and hopefully they'll stay this time around. 😅

@github-actions
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

baselines/dom.generated.d.ts Show resolved Hide resolved
baselines/dom.generated.d.ts Show resolved Hide resolved
baselines/webworker.generated.d.ts Show resolved Hide resolved
Copy link
Contributor

@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.

Cool, thanks! Some nits.

inputfiles/overridingTypes.jsonc Outdated Show resolved Hide resolved
baselines/audioworklet.generated.d.ts Show resolved Hide resolved
baselines/dom.generated.d.ts Show resolved Hide resolved
Copy link
Contributor Author

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

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

Unit tests are added.

I also added additional constructor signatures for ReadableStream, to distinguish between byte streams and non-byte streams. (This is a bit complex, so please take a look!)

baselines/webworker.generated.d.ts Show resolved Hide resolved
baselines/webworker.generated.d.ts Show resolved Hide resolved
Copy link
Contributor

@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.

Thank you!

(Sorry for responding late, I was off and playing games 😄. My vacation does not really apply here, but whatever.)

inputfiles/addedTypes.jsonc Outdated Show resolved Hide resolved
inputfiles/overridingTypes.jsonc Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Okay everything's cool, thank you for additional assertions!

controller.enqueue("b");
controller.close();
},
cancel(reason) {
assertType<any>(reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really do anything? 😄 (But probably good to be explicit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯

@saschanaz
Copy link
Contributor

LGTM

@github-actions github-actions bot merged commit 321faea into microsoft:main Aug 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

Merging because @saschanaz is a code-owner of all the changes - thanks!

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.

2 participants