-
Notifications
You must be signed in to change notification settings - Fork 317
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
Conversation
… webcomponents/polyfills.
…polyfill package instead.
📊 Tachometer Benchmark ResultsSummarycallback
Resultsshack
|
d6c3e83
to
64027e1
Compare
For some reason, I can't add @sorvell as a reviewer. |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
This PR removes the local types describing the polyfill APIs in favor of the new TS externs from the polyfill repos.