TypeScript-DOM-lib-generator Generic DOM Events

Hopefully fixes https://github.com/microsoft/TypeScript/issues/299

Added generics to Events so that target and currentTarget's types can be parameterized and event listeners can take a callback that expects an event targeting the receiver's type.

All new generics have default values, so the change should be backward compatible and all tests seem to be passing.

This is my first PR to the project and I hope I'm following the community norms, but please let me know if the implementation is up to standards or something is amiss.

Sep 29 '23 02:09 nscarcella

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.

Sep 29 '23 02:09 github-actions[bot]

@microsoft-github-policy-service agree

Sep 29 '23 02:09 nscarcella

But the main point is: is the performance issue solved?

I know it's not a perfect representation of the underlaying DOM as it is, but we're making perf vs memory vs accuracy trade-offs and I think we'll probably not take this.

https://github.com/microsoft/TypeScript-DOM-lib-generator/pull/969#issuecomment-784344870

Sep 29 '23 08:09 HolgerJeromin

But the main point is: is the performance issue solved?

@HolgerJeromin That is a good question. Not sure how to measure that. But the previous implementation was a very long time ago (I believe it was implemented on F#) and TypeScript has come a long way in terms of type performance since then, so I'm optimistic.

Maybe an admin can hint me how to check the performance is on acceptable levels?

Sep 29 '23 15:09 nscarcella

But the previous implementation was a very long time ago (I believe it was implemented on F#) and TypeScript has come a long way in terms of type performance since then, so I'm optimistic.

The resulting type is still same, the fact the generator was in F# doesn't mean much here. Whether the TypeScript itself has some performance improvement, that would be a better question. You should be able to pass a diagnostics flag to check it. https://github.com/microsoft/TypeScript/wiki/Performance#extendeddiagnostics

Sep 29 '23 19:09 saschanaz

@saschanaz should the extendeddiagnostics be run on my branch codebase or in a typescript project that uses it?

Here is the output of running it on my current branch:

> tsc --extendedDiagnostics

Files:                         181
Lines of Library:            38593
Lines of Definitions:        52870
Lines of TypeScript:          4211
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                107681
Symbols:                    113349
Types:                       56828
Instantiations:              55974
Memory used:               206653K
Assignability cache size:    23791
Identity cache size:           246
Subtype cache size:            289
Strict subtype cache size:     141
I/O Read time:               0.02s
Parse time:                  0.49s
ResolveModule time:          0.02s
ResolveTypeReference time:   0.00s
ResolveLibrary time:         0.01s
Program time:                0.57s
Bind time:                   0.22s
Check time:                  1.95s
transformTime time:          0.02s
Source Map time:             0.02s
commentTime time:            0.02s
I/O Write time:              0.01s
printTime time:              0.20s
Emit time:                   0.20s
Total time:                  2.93s

vs the one runned on main:

Files:                         181
Lines of Library:            38593
Lines of Definitions:        52870
Lines of TypeScript:          4211
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                107681
Symbols:                    113349
Types:                       56828
Instantiations:              55974
Memory used:               202933K
Assignability cache size:    23791
Identity cache size:           246
Subtype cache size:            289
Strict subtype cache size:     141
I/O Read time:               0.01s
Parse time:                  0.52s
ResolveModule time:          0.02s
ResolveTypeReference time:   0.00s
ResolveLibrary time:         0.01s
Program time:                0.60s
Bind time:                   0.22s
Check time:                  1.98s
transformTime time:          0.02s
Source Map time:             0.02s
commentTime time:            0.02s
I/O Write time:              0.00s
printTime time:              0.20s
Emit time:                   0.20s
Total time:                  2.99s

Sep 30 '23 15:09 nscarcella

should the extendeddiagnostics be run on my branch codebase or in a typescript project that uses it?

Both, because otherwise a comparison can't be made 😁

Sep 30 '23 15:09 saschanaz

Which command are you using btw?

Sep 30 '23 15:09 saschanaz

@saschanaz

Which command are you using btw?

tsc --extendedDiagnostics

Sep 30 '23 18:09 nscarcella

And any more arguments? Last time I think I passed --noLib built/dom.generated.d.ts on top of that.

Sep 30 '23 19:09 saschanaz

@saschanaz running both main and mine branch with that flag gives a few errors (sorry, i'm not very fluent in special settings of tsc) but yields these results:

main

> tsc --extendedDiagnostics --noLib generated/dom.generated.d.ts

error TS2318: Cannot find global type 'Boolean'.

error TS2318: Cannot find global type 'Function'.

error TS2318: Cannot find global type 'IArguments'.

error TS2318: Cannot find global type 'Number'.

error TS2318: Cannot find global type 'Object'.

error TS2318: Cannot find global type 'RegExp'.


Found 6 errors.

Files:                         103
Lines of Library:                0
Lines of Definitions:        72881
Lines of TypeScript:             0
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                 72154
Symbols:                     83601
Types:                       37109
Instantiations:              40266
Memory used:               162580K
Assignability cache size:    16188
Identity cache size:             1
Subtype cache size:              0
Strict subtype cache size:       0
I/O Read time:               0.01s
Parse time:                  0.43s
ResolveTypeReference time:   0.01s
ResolveModule time:          0.01s
Program time:                0.48s
Bind time:                   0.16s
Check time:                  1.54s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  2.18s

generic-dom:

> tsc --extendedDiagnostics --noLib generated/dom.generated.d.ts

error TS2318: Cannot find global type 'Boolean'.

error TS2318: Cannot find global type 'Function'.

error TS2318: Cannot find global type 'IArguments'.

error TS2318: Cannot find global type 'Number'.

error TS2318: Cannot find global type 'Object'.

error TS2318: Cannot find global type 'RegExp'.


Found 6 errors.

Files:                         103
Lines of Library:                0
Lines of Definitions:        72881
Lines of TypeScript:             0
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                 72154
Symbols:                     83601
Types:                       37109
Instantiations:              40266
Memory used:               162606K
Assignability cache size:    16188
Identity cache size:             1
Subtype cache size:              0
Strict subtype cache size:       0
I/O Read time:               0.01s
Parse time:                  0.49s
ResolveTypeReference time:   0.01s
ResolveModule time:          0.01s
Program time:                0.54s
Bind time:                   0.17s
Check time:                  1.54s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  2.24s

Sep 30 '23 20:09 nscarcella

Did you run npm run build to generate built/dom.generated.d.ts before the command?

Sep 30 '23 20:09 saschanaz

@saschanaz I did run npm run build, but no built folder is created. There is a generated folder, and I assumed that was the one I should use, so I ran this on both branches:

tsc --extendedDiagnostics --noLib generated/dom.generated.d.ts

Was this OK or did I do something wrong?

Sep 30 '23 20:09 nscarcella

The result is strange because it seems nothing is different, and that shouldn't be the case?

Sep 30 '23 20:09 saschanaz

The result is strange because it seems nothing is different, and that shouldn't be the case?

Do you think maybe someone could double check in case I'm goofing somewhere?

Sep 30 '23 20:09 nscarcella

@saschanaz my tests running on a project importing this branch as dependency yields similar results. Not sure how to continue from here

Oct 04 '23 22:10 nscarcella

These changes are a great improvement, but adding the same type to currentTarget and target is not a good idea as they can be different. Actually I think only currentTarget can be properly typed this way. The target property can be any HTMLElement or Node?

Oct 13 '23 06:10 DaSchTour

@DaSchTour After learning a bit more about target, relatedTarget and currentTarget I see you are right. I don't mind making the relevant changes but I would very much like to get some feedback about the performance before, to avoid spending time on the taskif it's not good enough to merge.

Oct 15 '23 23:10 nscarcella

Really hoping to see this PR prioritized and moved along soon - a month without feedback on this issue is too long! this is one of the oldest and most frustrating issues in TypeScript - it literally impacts every browser based project, and it's probably a big reason why some people still feel TS "gets in their way".

Dealing with events is so common and just should not be the stumbling block it is today, frankly, performance be damned - if having the correct types for something as essential as browser events drastically impacts performance, then the team should prioritize solving that next.

"make it work, make it right, make it fast" - please let's move past step one of three.

Projects that would be substantially impacted by any performance issue with generic events already are, as they're just building it themselves. (React, etc.)

At the very least, please consider making this available as a separate, opt-in standard type library, so teams can decide for themselves what's more important or where they feel their time is best spent.

After 10+ years of waiting, please do something to move forward this time. 🙏

Nov 18 '23 07:11 mindplay-dk

Finally got some time to take a look. Without this patch:

> tsc --diagnostics --lib es5 baselines/dom.generated.d.ts
Files:              173
Lines:            86953
Nodes:           249884
Identifiers:      84472
Symbols:         103446
Types:            50819
Instantiations:   58489
Memory used:    178163K
I/O read:         0.04s
I/O write:        0.00s
Parse time:       0.74s
Bind time:        0.20s
Check time:       1.62s
Emit time:        0.00s
Total time:       2.56s

With this patch: (ignore the times, those are not very stable)

> tsc --diagnostics --lib es5 baselines/dom.generated.d.ts
Files:              173
Lines:            86953
Nodes:           250018
Identifiers:      84538
Symbols:         105506
Types:            51165
Instantiations:   59466
Memory used:    181471K
I/O read:         0.05s
I/O write:        0.00s
Parse time:       1.15s
Bind time:        0.29s
Check time:       2.21s
Emit time:        0.00s
Total time:       3.65s

Difference exists, but not too much compared to how much the community wants this feature. This kind of performance difference can absolutely happen when more types are added e.g. in #1514. @sandersn, does this look okay for you?

But this patch can't be merged as-is:

target should remain to be EventTarget, as it can have a different type. For example:
```
<picture onclick="console.log(event.currentTarget); console.log(event.target)">
  <button>Click me</button>
</picture>
```
Clicking the button will give currentTarget being HTMLPictureElement and target being HTMLButtonElement.
extends Event<T> should rather be autogenerated rather than manually overridden by overridingTypes.jsonc.

Dec 17 '23 11:12 saschanaz

BTW, this patch does not cover onevent properties, would that make a huge difference? We'll see 🙂 (it should be in this patch, otherwise it'll break consistency)

Dec 17 '23 11:12 saschanaz

Hey @saschanaz! I updated the PR with the following changes:

reverted type of target back to EventTarget. Now so that currentTarget is the only property using the parameterized type.
Moved the Event definitions to addedTypes (not sure if this was what you meant in the point (2).

Regarding the onevent properties, my understanding was that it should be possible to assign those even if the parametric type remains unspecified on the declaration because variance is not checked (I made a small test here: here). Since compilation time seems critical I thought leaving those be would be the safer approach.

Let me know your thoughts.

Feb 02 '24 17:02 nscarcella

Every *Types.json is manual, autogeneration happens from https://github.com/microsoft/TypeScript-DOM-lib-generator/blob/main/src/build/emitter.ts. Specifically I think this part can be modified for events:

https://github.com/microsoft/TypeScript-DOM-lib-generator/blob/9524e547d132a20df7a01c81e421ed245b5b9618/src/build/emitter.ts#L1187-L1219

Regarding the onevent properties, my understanding was that it should be possible to assign those even if the parametric type remains unspecified on the declaration because variance is not checked

My point is:

const input = document.querySelector("input");

input.onchange = ev => {
    ev.currentTarget; // Still EventTarget, we want HTMLInputElement
}

My suggested work items:

Let's autogenerate from emitter instead of manually overriding for every event interface
We need to cover onevent
We also need to cover other addEventListeners, this patch currently only covers EventTarget.addEventListener which is always overridden by subclasses and thus wouldn't be very useful
There should be some unit test, see https://github.com/microsoft/TypeScript-DOM-lib-generator/tree/main/unittests/files

Feb 02 '24 21:02 saschanaz

Hi @saschanaz I made some time to check the code and moved the extends declarations to the emiter logic (instead of overriding it on the .json files).

Does this implementation go along the lines you had in mind? Do you think the onevent methods should be handled in a similar way?

Feb 22 '24 20:02 nscarcella