Open andria-dev opened 1 month ago
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.
@microsoft-github-policy-service agree
Have you read and fixed the issues from this item? https://github.com/microsoft/TypeScript/issues/283#issuecomment-608032921
@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.
Manual overridding was a workaround to reduce performance problem: https://github.com/microsoft/TypeScript-DOM-lib-generator/pull/220#discussion_r108237579
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.
You can run tsc --diagnostics --lib es5 baselines/dom.generated.d.ts
and see how the numbers increase before and after the patch 👍
@saschanaz I took 50 samples before and after the change and averaged each sample set.
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.
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 }
@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).
@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.
The
cloneNode
return type is incorrect as per #283, #302, https://github.com/microsoft/TypeScript/issues/17818 and #1578. The type before this PR indicated thatcloneNode
returned aNode
, which then broke any class that extendedNode
. In reality,cloneNode
will return an instance of whatever the current class type is, a.k.a.this
(DocumentFragment#cloneNode
returns aDocumentFragment
, not aNode
).This PR solves this issue by changing the return type of
Node#cloneNode
tothis
.