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

Use BCD to autoremove unimplemented features #915

Merged
merged 20 commits into from
Mar 4, 2021

Conversation

saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Sep 16, 2020

This patch focuses only on removing totally unimplemented features, and defers everything else:

  • features only with single implementation should be reevaluated later
  • features that are widely-supported but over-restricted by removedTypes.json should also be reevaluated later.

forceKeepAlive is quite huge now but it eventually will be removed once reevaluation ends.

@@ -2737,29 +2708,6 @@ declare var ReadableStream: {
new<R = any>(underlyingSource?: UnderlyingSource<R>, strategy?: QueuingStrategy<R>): ReadableStream<R>;
};

interface ReadableStreamBYOBReader {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MattiasBuelens, could you review the Streams part of this removal? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

UnderlyingByteSource, ReadableByteStreamControllerCallback and this ReadableStream constructor overload should be removed as well.

You might want to have a look at 361a870 (part of #890).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hang on, where did WritableStream and TransformStream go? There are still types referencing them (like GenericTransformStream and ReadableStream.pipeTo).

(I understand why they would need to be removed, but it still makes me sad to see them go... 😞)

Copy link
Contributor Author

@saschanaz saschanaz Sep 18, 2020

Choose a reason for hiding this comment

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

The bright side is that this patch only disables some of your work but not removes them all. It can be enabled again when browsers implement them 👍

(Disabling them should also boost #890 👀👀)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, WritableStream and TransformStream are still alive as there is at least a single implementation and thus not covered here. This patch is intentionally only removes totally unimplemented ones to reduce the diff size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Streams changes LGTM. 👍 Happy to see WritableStream and TransformStream are still alive! 😁

I think it's best to wait with landing #890 until after this PR lands. I'll see if I can rebase my PR to not completely remove the readable byte stream definitions, and instead rely on your work to auto-remove them. 🙂

@saschanaz
Copy link
Contributor Author

Oops, test fails 👀

@saschanaz saschanaz marked this pull request as draft September 17, 2020 21:21
@saschanaz saschanaz marked this pull request as ready for review September 18, 2020 08:14
@foolip
Copy link

foolip commented Sep 22, 2020

@vinyldarkscratch this PR is of interest to our ongoing project. In particular the list of BCD bugs in #915 (comment) are things that can hopefully be fixed by using https://github.com/foolip/mdn-bcd-collector. Where they can't, that might reveal a deficiency in our approach.

@saschanaz
Copy link
Contributor Author

Changes are too big, maybe better to remove things progressively. I'll make separate PRs.

@sandersn
Copy link
Member

@orta and I like this idea -- in fact, we'd rather take this PR than the dozen or so individual-removal PRs.

  • Do they remove the same set of declarations? It looks that way.
  • Is this PR ready to go otherwise? I've only scanned the code, but it looks OK at that level of detail.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Approving the code change -- couple of open questions about the resulting DOM changes.

@saschanaz saschanaz marked this pull request as ready for review February 27, 2021 21:14
@saschanaz
Copy link
Contributor Author

saschanaz commented Feb 27, 2021

@orta and I like this idea -- in fact, we'd rather take this PR than the dozen or so individual-removal PRs.

  • Do they remove the same set of declarations? It looks that way.

  • Is this PR ready to go otherwise? I've only scanned the code, but it looks OK at that level of detail.

  • Yes, they are already merged in this PR.
  • Also yes. It's using older version of BCD since the PR has been rotting, though.

@orta
Copy link
Contributor

orta commented Mar 1, 2021

Cool, IMO we should let's get this in and into the compiler then 👍🏻

@sandersn sandersn merged commit f45dc9f into microsoft:master Mar 4, 2021
@sandersn
Copy link
Member

sandersn commented Mar 4, 2021

I'm going to port this over to Typescript right now and will tag you (@orta and @saschanaz) on the PR when it's ready.

@sandersn
Copy link
Member

sandersn commented Mar 4, 2021

OK, it is merged into TS now.

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.

6 participants