cloneNode Return Type Must Be Active Class, Not Base Class (Node)

[andria-dev]

The cloneNode return type is incorrect as per #283, #302, microsoft/TypeScript#17818 and #1578. The type before this PR indicated that cloneNode returned a Node, which then broke any class that extended Node. In reality, cloneNode will return an instance of whatever the current class type is, a.k.a. this (DocumentFragment#cloneNode returns a DocumentFragment, not a Node).

This PR solves this issue by changing the return type of Node#cloneNode to this.

[github-actions]

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

[HolgerJeromin]

Have you read and fixed the issues from this item?
microsoft/TypeScript#283 (comment)

[andria-dev]

@HolgerJeromin, I will test that later today. But that issue should be fixed as we aren't manually overriding each subclass'es cloneNode return type anymore, just the original Node#cloneNode return type.

[saschanaz]

Manual overridding was a workaround to reduce performance problem: #220 (comment)

[andria-dev]

Ah, my mistake, @saschanaz. I missed that. If there's no possible workaround that is both performant and covariant, what is the project's focus, performance or accuracy? Making the change to this locally works fine for me performance-wise, but I'm not exactly running on a low-end computer either.

[saschanaz]

You can run tsc --diagnostics --lib es5 baselines/dom.generated.d.ts and see how the numbers increase before and after the patch 👍

[andria-dev]

@saschanaz I took 50 samples before and after the change and averaged each sample set.

Results

Version	Files	Lines	Identifiers	Symbols	Types	Instantiations	Memory used (Kilobytes)	I/O read (seconds)	I/O write (seconds)	Parse time (seconds)	Bind time (seconds)	Check time (seconds)	Emit time (seconds)	Total time (seconds)
Before	177	89627	78827	95960	44887	56733	164941.94	0.021	0	0.3876	0.1182	0.9082	0	1.4132
After	177	89627	78825	96281	45038	57035	166090.88	0.022	0	0.3862	0.1168	0.912	0	1.4172

So it looks like an average total increase of 4 milliseconds on my system.

Methodology

The results were aggregated with some PowerShell. I ran the following script under the main branch, and then I changed before.csv to after.csv and ran it under this branch. After that, I used LibreOffice Calc's AVERAGE function to calculate the averages of each set of diagnostic runs. I made sure I wasn't doing anything on my computer during either set so that they were as consistent as possible.

Write-Output "Files,Lines,Identifiers,Symbols,Types,Instantiations,Memory used,I/O read,I/O write,Parse time,Bind time,Check time,Emit time,Total time" | Out-File -FilePath ..\before.csv; 1..50 | ForEach-Object { (npx tsc --diagnostics --lib es5 baselines/dom.generated.d.ts | ForEach-Object { ($_ -Split ':')[1].Trim() -ireplace '(s|k)' }) -Split "`n" -Join "," | Out-File -Append -FilePath ..\before.csv }

[andria-dev]

@HolgerJeromin I did a little reproduction, and it looks like HTMLElement type parameters are still covariant with this change.

// Same type parameter as Tablesorter.
interface A<T = HTMLElement> {
	x?: T;
}
type An<T = HTMLElement> = A<T>;

type ANode = A<Node>;
type AnElement = An<Element>;
type AnHTMLElement = An;
type AnHTMLAnchorElement = An<HTMLAnchorElement>;

let node: ANode = {};
let element: AnElement = {};
let htmlElement: AnHTMLElement = {};
let htmlAnchorElement: AnHTMLAnchorElement = {};

// All of the return types are correct!
let clonedNode: Node = node.x?.cloneNode()!;
let clonedElement: Element = element.x?.cloneNode()!;
let clonedHtmlElement: HTMLElement = htmlElement.x?.cloneNode()!;
let clonedHtmlAnchorElement: HTMLAnchorElement = htmlAnchorElement.x?.cloneNode()!;

htmlElement = htmlAnchorElement; // Covariance works! 👍🏻
htmlElement = element; // Contravariance doesn't work (as expected).

[andria-dev]

@saschanaz Just wanted to follow up with a ping. This was my conclusion:

So it looks like an average total increase of 4 milliseconds on my system.

A 4 millisecond difference seems much more acceptable than the previous 700 millisecond difference. If you have any other concerns please let me know.

[callionica]

I'm very happy to see work done on this and since it looks like perf impact is minimal, hope this will get merged soon. This would be a huge improvement for front end devs using typescript.