Skip to content

Replace local type declarations for polyfill APIs with those imported from the polyfills. #1208

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

Merged
merged 5 commits into from
Aug 6, 2021

Conversation

bicknellr
Copy link
Member

@bicknellr bicknellr commented Jul 26, 2021

This PR removes the local types describing the polyfill APIs in favor of the new TS externs from the polyfill repos.

@google-cla google-cla bot added the cla: yes label Jul 26, 2021
@github-actions
Copy link

github-actions bot commented Jul 26, 2021

📊 Tachometer Benchmark Results

Summary

callback

  • shack: unsure 🔍 -1% - +2% (-0.23ms - +0.57ms)
    this-change vs master

Results

shack
VersionAvg timevs this-changevs mastervs previous-2.x-release
this-change24.66ms - 25.20ms-unsure 🔍
-1% - +2%
-0.23ms - +0.57ms
unsure 🔍
-0% - +2%
-0.08ms - +0.61ms
master24.47ms - 25.06msunsure 🔍
-2% - +1%
-0.57ms - +0.23ms
-unsure 🔍
-1% - +2%
-0.27ms - +0.46ms
previous-2.x-release24.46ms - 24.88msunsure 🔍
-2% - +0%
-0.61ms - +0.08ms
unsure 🔍
-2% - +1%
-0.46ms - +0.27ms
-

tachometer-reporter-action v2 for Benchmarks

@bicknellr bicknellr force-pushed the polyfill-ts-externs branch from d6c3e83 to 64027e1 Compare July 27, 2021 21:30
@bicknellr bicknellr marked this pull request as ready for review July 28, 2021 19:45
@bicknellr
Copy link
Member Author

For some reason, I can't add @sorvell as a reviewer.

Copy link
Member

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

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

LGTM, just the one question about ShadowRoot.

interface Window {
ShadyCSS?: ShadyCSS;
ShadyDOM?: ShadyDOM;
ShadowRoot: typeof ShadowRoot;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we still need ShadowRoot on window? Seems like that's in lib.dom.d.ts...

Copy link
Member Author

Choose a reason for hiding this comment

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

I was limiting the PR to only removing the parts that are now provided externally, but it builds without a lot of other stuff in here. Should I remove those also?

Copy link
Member Author

Choose a reason for hiding this comment

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

(FWIW, this declaration of ShadowRoot is not necesary, but the part below that adds adoptedStyleSheets still is.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think I'd rather wait to clean unrelated extra declarations out of here in another PR.

Copy link
Member Author

@bicknellr bicknellr Aug 5, 2021

Choose a reason for hiding this comment

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

Side note: Now that I think about it, lit/lit#1904 was probably a mistake that will become repairable once this PR (edit: and a PR removing the other unnecessary parts) is in. Originally, I found that those types were conflicting internally with something (the original JS externs for the polyfills, I think?) so that PR avoided the problem by not adding to Window. Now that the polyfill types all have exactly one definition, it might be better to go back to adding Lit-specific special properties to Window because they are actually there and a collision would indicate a real problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

(We talked in a meeting about this, I'm going to go ahead and merge.)

@bicknellr bicknellr merged commit 8e32c37 into master Aug 6, 2021
@bicknellr bicknellr deleted the polyfill-ts-externs branch August 6, 2021 17:32
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.

2 participants