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

Add cloneNode overloads #811

Merged
merged 3 commits into from
Mar 17, 2020
Merged

Conversation

IllusionMH
Copy link
Contributor

@IllusionMH IllusionMH commented Dec 9, 2019

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 like attachEventListener so I decided to add cloneNode too. Should I remove cloneNode from them?

ShadowRoot has special treatment because cloneNode throws when called on a shadow root. Should I leave never 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.

@sandersn sandersn self-requested a review as a code owner March 17, 2020 16:43
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.

This looks good. Please do share your script when you can; I've brought this up to date and will merge it shortly.

@sandersn sandersn merged commit 17d9039 into microsoft:master Mar 17, 2020
@@ -2239,7 +2239,7 @@
"cloneNode": {
"name": "cloneNode",
"override-signatures": [
"cloneNode(deep?: boolean): HTMLDialogElement"
"cloneNode(deep?: boolean): HTMLVideoElement"
Copy link
Contributor Author

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.

@saschanaz
Copy link
Contributor

Is this any better than cloneNode<this>?

@IllusionMH
Copy link
Contributor Author

IFAIK it was impossible to use this in for cloneNode declaration inside of Node because it would make it generic and result in performance hit #220 (comment) .
There were attempts to use this earlier #302 (comment) .

@saschanaz
Copy link
Contributor

saschanaz commented Mar 17, 2020

That's true, and I mean, are we sure this is performance wise better than making it generic?

Edit: See also #166

@saschanaz
Copy link
Contributor

Ran the diagnostics and the answers seems yes 👍

Before:

> npx tsc --diagnostics baselines/dom.generated.d.ts --lib es5
Files:              29
Lines:           39039
Nodes:          175930
Identifiers:     62248
Symbols:         51857
Types:           14117
Instantiations:   7923
Memory used:    92593K
I/O read:        0.01s
I/O write:       0.00s
Parse time:      0.55s
Bind time:       0.27s
Check time:      1.02s
Emit time:       0.00s
Total time:      1.84s

After:

> npx tsc --diagnostics baselines/dom.generated.d.ts --lib es5
Files:              29
Lines:           39190
Nodes:          177314
Identifiers:     62870
Symbols:         52163
Types:           14261
Instantiations:   7923
Memory used:    94520K
I/O read:        0.01s
I/O write:       0.00s
Parse time:      0.56s
Bind time:       0.25s
Check time:      1.01s
Emit time:       0.00s
Total time:      1.82s

No significant difference, while somehow it's 2x heavier than it was 4 years ago 👀.

@IllusionMH
Copy link
Contributor Author

Before vs After means: before vs after this PR or with 160 new overloads vs using this?

@saschanaz
Copy link
Contributor

Before vs after this PR.

Using this:

Files:               29
Lines:            39039
Nodes:           175928
Identifiers:      62246
Symbols:          76776
Types:            23867
Instantiations:   45513
Memory used:    117031K
I/O read:         0.01s
I/O write:        0.00s
Parse time:       0.56s
Bind time:        0.26s
Check time:       1.21s
Emit time:        0.00s
Total time:       2.02s

Significantly more memory use.

@saschanaz
Copy link
Contributor

BTW, maybe merge the script used for this PR to the build script so that it can be automated for whatever new element types?

@IllusionMH IllusionMH deleted the clonenode-overloads branch March 17, 2020 23:27
@IllusionMH
Copy link
Contributor Author

@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 cloneNode for custom components? Because with this approach cloneNode would return type of built-in component they inherit instead of their own class.

@jcubic
Copy link

jcubic commented Dec 25, 2023

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:

Type 'Node' is missing the following properties from type 'Element': attributes, classList, className, clientHeight, and 114 more.

Was it removed?

@HolgerJeromin
Copy link
Contributor

Removed in #842

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.

cloneNode should return sub type, not Node
5 participants