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

chore: update wpt #9907

Merged
merged 1 commit into from
Mar 27, 2021
Merged

chore: update wpt #9907

merged 1 commit into from
Mar 27, 2021

Conversation

crowlKats
Copy link
Member

@crowlKats crowlKats commented Mar 26, 2021

tests that are failing that are actionable:
ReadableStream: if pull rejects, it should error the stream
TransformStream constructor should not call setters for highWaterMark or size
Required for #7819

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@lucacasonato lucacasonato merged commit b112496 into denoland:main Mar 27, 2021
@crowlKats crowlKats deleted the update_wpt branch March 27, 2021 15:33
kitsonk pushed a commit that referenced this pull request Apr 1, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Apr 1, 2021

We didn't add a unit test for this, which didn't catch that we didn't update the typings for this. We also had #9777 open (without CI passing).

From a stylistic perspective, we should put static methods at the end of the class definition.

Can we follow this up with an update to the lib.deno_web.d.ts?

@lucacasonato
Copy link
Member

We didn't add a unit test for this

Yes we did - WPT tests it.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 1, 2021

We didn't add a unit test for this
Yes we did - WPT tests it.

But it doesn't test the types... and because we don't author in TypeScript anymore, we have no clue that we missed updating the types. 🤷

The following errors with type checking on:

AbortSignal.abort()

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.

3 participants