-
Notifications
You must be signed in to change notification settings - Fork 431
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 cloneNode overloads #811
Conversation
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.
This looks good. Please do share your script when you can; I've brought this up to date and will merge it shortly.
@@ -2239,7 +2239,7 @@ | |||
"cloneNode": { | |||
"name": "cloneNode", | |||
"override-signatures": [ | |||
"cloneNode(deep?: boolean): HTMLDialogElement" | |||
"cloneNode(deep?: boolean): HTMLVideoElement" |
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.
Not sure where this one came from probably from conflict.
Thanks for fixing this one.
Is this any better than |
IFAIK it was impossible to use |
That's true, and I mean, are we sure this is performance wise better than making it generic? Edit: See also #166 |
Ran the diagnostics and the answers seems yes 👍 Before:
After:
No significant difference, while somehow it's 2x heavier than it was 4 years ago 👀. |
Before vs After means: before vs after this PR or with 160 new overloads vs using |
Before vs after this PR. Using
Significantly more memory use. |
BTW, maybe merge the script used for this PR to the build script so that it can be automated for whatever new element types? |
@sandersn @saschanaz you both should receive notification from git with code (which is definitely a mess that cannot be included in this repo). Can you please suggest where I can add info on how to properly type this |
How this is fixed? I still need to use this code: let style = target.querySelector('style');
if (!style) {
style = $style.cloneNode() as typeof $style;
} Otherwise, I got an error:
Was it removed? |
Removed in #842 |
Fixes microsoft/TypeScript#283
This PR adds overloads to almost each interface that has
Node
in its ancestors.Only interface that was skipped is
ChildNode
because this is mixing interface and shouldn't observed.There are several interfaces that regular DOM operations shouldn't return (e.g.
SVGAnimationElement
,SVGComponentTransferFunctionElement
) because return types will be concrete interfaces that extend these. Moreover these clases has other methods likeattachEventListener
so I decided to addcloneNode
too. Should I removecloneNode
from them?ShadowRoot
has special treatment becausecloneNode
throws when called on a shadow root. Should I leavenever
as return type or what type should I use there?Those who create custom components need to add type to their own overload of
cloneNode
.If needed I can share script I used for this change which is based on baselines for simplicity.