Generic DOM Events #1624

nscarcella · 2023-09-29T02:50:56Z

Hopefully fixes microsoft/TypeScript#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.

github-actions · 2023-09-29T02:51:12Z

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.

nscarcella · 2023-09-29T02:52:13Z

@microsoft-github-policy-service agree

inputfiles/overridingTypes.jsonc

HolgerJeromin · 2023-09-29T08:04:07Z

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.

#969 (comment)

nscarcella · 2023-09-29T15:29:53Z

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?

saschanaz · 2023-09-29T19:07:00Z

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

nscarcella · 2023-09-30T15:42:27Z

@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

saschanaz · 2023-09-30T15:47:59Z

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 😁

saschanaz · 2023-09-30T15:49:17Z

Which command are you using btw?

nscarcella · 2023-09-30T18:18:06Z

@saschanaz

Which command are you using btw?

tsc --extendedDiagnostics

saschanaz · 2023-09-30T19:56:05Z

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

nscarcella · 2023-09-30T20:04:43Z

@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

saschanaz · 2023-09-30T20:16:54Z

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

nscarcella · 2023-09-30T20:28:59Z

@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?

saschanaz · 2023-09-30T20:32:24Z

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

nscarcella · 2023-09-30T20:39:27Z

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?

nscarcella · 2023-10-04T22:21:26Z

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

DaSchTour · 2023-10-13T06:29:12Z

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?

nscarcella · 2023-10-15T23:35:05Z

@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.

mindplay-dk · 2023-11-18T07:49:51Z

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. 🙏

saschanaz · 2023-12-17T11:26:51Z

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.

saschanaz · 2023-12-17T11:40:21Z

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)

nscarcella · 2024-02-02T17:56:34Z

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.

saschanaz · 2024-02-02T21:00:52Z

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:

TypeScript-DOM-lib-generator/src/build/emitter.ts

Lines 1187 to 1219 in 9524e54

    
             function emitInterfaceDeclaration(i: Browser.Interface) { 
        
               function processIName(iName: string) { 
        
                 return extendConflictsBaseTypes[iName] ? `${iName}Base` : iName; 
        
               } 
        
               const processedIName = processIName(i.name); 
        
               if (processedIName !== i.name) { 
        
                 printer.printLineToStack( 
        
                   `interface ${getNameWithTypeParameter( 
        
                     i.typeParameters, 
        
                     i.name, 
        
                   )} extends ${processedIName} {`, 
        
                 ); 
        
               } 
        
               emitComments(i, printer.printLine); 
        
               printer.print( 
        
                 `interface ${getNameWithTypeParameter(i.typeParameters, processedIName)}`, 
        
               ); 
        
               const finalExtends = [i.extends || "Object"] 
        
                 .concat(getImplementList(i.name)) 
        
                 .filter((i) => i !== "Object") 
        
                 .map(processIName); 
        
               if (finalExtends.length) { 
        
                 printer.print(` extends ${assertUnique(finalExtends).join(", ")}`); 
        
               } 
        
               printer.print(" {"); 
        
               printer.endLine(); 
        
             }

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

…ding JSON

nscarcella · 2024-02-22T20:40:20Z

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?

saschanaz

This certainly looks better, have you compared performance with the latest changes?

saschanaz · 2024-02-22T21:04:57Z

src/build/helpers.ts

@@ -62,6 +62,8 @@ export const baseTypeConversionMap = new Map<string, string>([
  ["EventHandler", "EventHandler"],
 ]);

+export const genericEventSupertypes = new Set(["UIEvent", "MouseEvent"]);


I expected this to cover every event, why should it be limited?

It will allow to support dispatched events in future (just idea).

const event /* : MouseEvent<never> */ = new MouseEvent("click") button.addEventListener("click", (event /* : MouseEvent<HTMLButtonElement> */) => {})

Also important - CustomEvent, MessageEvent and ProgressEvent already use T type parameter. Could we use other type parameter name for currentTarget?

@saschanaz

I expected this to cover every event, why should it be limited?

I just thought it might be useful to be able to limit the implementation to DOM events in case performance became an issue, but I guess we could apply this to every Event subtype, if you think it is better.

@turansky

CustomEvent, MessageEvent and ProgressEvent already use T type parameter. Could we use other type parameter name for currentTarget?

I was being conservative about the change, so the current implementation only takes interfaces that have no type parameter defined (I made it so there would not be unexpected conflicts) so, for those types we could override on the json files. But yes, we could instead always use another name if that seems better. Does something like Target sound good? I'm not sure how opinionated is the project regarding names.

Updated request - use T for currentTarget type parameter in all events.

For ProgressEvent it will be common type parameter for both targets (now it's used for target only).

For MessgegeEvent.data and CustomEvent.details we can use D as type parameter name.

In BaseSyntheticEvent from @types/react
C - currentTarget
T - target

I just thought it might be useful to be able to limit the implementation to DOM events in case performance became an issue, but I guess we could apply this to every Event subtype, if you think it is better.

We should first check the difference and then decide based on that.

@turansky @saschanaz Ok, I'll see to handle all events instead of whitelisting and will rename the mentioned type parameters to D.

In the meantime, I updated the PR with a possible approach to type the onevent handlers for the currently handled interfaces.

@nscarcella FYI - I implemented similar changes in Kotlin Wrappers.
I used followings script:

Remove T type parameter from ProgressEvent

To replace it later with common C

D type parameter for CustomEvent and MessageEvent

T reserved as future type parameter for target (will work fine in handlers like FileReader.onload)

C - type parameter for currentTarget for all events

In Kotlin we also received strict dispatched events (with nonnull currentTarget) as bonus.
In TypeScript it can be solved in separate PR.

nscarcella · 2024-02-23T21:09:31Z

src/build/emitter.ts

@@ -1186,6 +1207,8 @@ export function emitWebIdl(

  function emitInterfaceDeclaration(i: Browser.Interface) {
    function processIName(iName: string) {
+      if (iName === "GlobalEventHandlers" && i.name !== iName)


Not sure if this is the right approach or this should be declared on the json files.

Hopefully fixes microsoft#299

acd8fc2

HolgerJeromin reviewed Sep 29, 2023

View reviewed changes

inputfiles/overridingTypes.jsonc Outdated Show resolved Hide resolved

HolgerJeromin reviewed Sep 29, 2023

View reviewed changes

inputfiles/overridingTypes.jsonc Show resolved Hide resolved

Cleanup

8f85aaa

nscarcella mentioned this pull request Oct 12, 2023

Request to change currentTarget in Event interface for lib.d.ts microsoft/TypeScript#299

Open

sandersn mentioned this pull request Jan 12, 2024

Test generic DOM events again microsoft/TypeScript#57040

Open

nscarcella added 2 commits February 2, 2024 13:08

reverting type of target to EventTarget

cabf7f7

Moved Event definitions to addedTypes

d618a12

nscarcella added 2 commits February 22, 2024 17:19

Generating extension of generic event types by code instead of overri…

96fd961

…ding JSON

Merge branch 'main' into genericDomEvents

898bad6

nscarcella force-pushed the genericDomEvents branch from 03b3857 to 898bad6 Compare February 22, 2024 20:30

Fixing linter

83982b7

saschanaz reviewed Feb 22, 2024

View reviewed changes

(draft) adding handing of onevent

2a2157e

nscarcella marked this pull request as draft February 23, 2024 21:05

nscarcella commented Feb 23, 2024

View reviewed changes

turansky mentioned this pull request Feb 24, 2024

Events. Type parameter for Event.currentTarget JetBrains/kotlin-wrappers#2217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic DOM Events #1624

Generic DOM Events #1624

nscarcella commented Sep 29, 2023 •

edited

github-actions bot commented Sep 29, 2023

nscarcella commented Sep 29, 2023 •

edited

HolgerJeromin commented Sep 29, 2023

nscarcella commented Sep 29, 2023 •

edited

saschanaz commented Sep 29, 2023

nscarcella commented Sep 30, 2023 •

edited

saschanaz commented Sep 30, 2023

saschanaz commented Sep 30, 2023

nscarcella commented Sep 30, 2023

saschanaz commented Sep 30, 2023

nscarcella commented Sep 30, 2023

saschanaz commented Sep 30, 2023 •

edited

nscarcella commented Sep 30, 2023

saschanaz commented Sep 30, 2023

nscarcella commented Sep 30, 2023

nscarcella commented Oct 4, 2023

DaSchTour commented Oct 13, 2023

nscarcella commented Oct 15, 2023

mindplay-dk commented Nov 18, 2023

saschanaz commented Dec 17, 2023 •

edited

saschanaz commented Dec 17, 2023 •

edited

nscarcella commented Feb 2, 2024

saschanaz commented Feb 2, 2024

nscarcella commented Feb 22, 2024

saschanaz left a comment

saschanaz Feb 22, 2024 •

edited

turansky Feb 22, 2024 •

edited

nscarcella Feb 23, 2024

turansky Feb 23, 2024

turansky Feb 23, 2024

saschanaz Feb 23, 2024

nscarcella Feb 23, 2024

turansky Feb 28, 2024 •

edited

nscarcella Feb 23, 2024

Generic DOM Events #1624

Are you sure you want to change the base?

Generic DOM Events #1624

Conversation

nscarcella commented Sep 29, 2023 • edited

github-actions bot commented Sep 29, 2023

nscarcella commented Sep 29, 2023 • edited

HolgerJeromin commented Sep 29, 2023

nscarcella commented Sep 29, 2023 • edited

saschanaz commented Sep 29, 2023

nscarcella commented Sep 30, 2023 • edited

saschanaz commented Sep 30, 2023

saschanaz commented Sep 30, 2023

nscarcella commented Sep 30, 2023

saschanaz commented Sep 30, 2023

nscarcella commented Sep 30, 2023

saschanaz commented Sep 30, 2023 • edited

nscarcella commented Sep 30, 2023

saschanaz commented Sep 30, 2023

nscarcella commented Sep 30, 2023

nscarcella commented Oct 4, 2023

DaSchTour commented Oct 13, 2023

nscarcella commented Oct 15, 2023

mindplay-dk commented Nov 18, 2023

saschanaz commented Dec 17, 2023 • edited

saschanaz commented Dec 17, 2023 • edited

nscarcella commented Feb 2, 2024

saschanaz commented Feb 2, 2024

nscarcella commented Feb 22, 2024

saschanaz left a comment

Choose a reason for hiding this comment

saschanaz Feb 22, 2024 • edited

Choose a reason for hiding this comment

turansky Feb 22, 2024 • edited

Choose a reason for hiding this comment

nscarcella Feb 23, 2024

Choose a reason for hiding this comment

turansky Feb 23, 2024

Choose a reason for hiding this comment

turansky Feb 23, 2024

Choose a reason for hiding this comment

saschanaz Feb 23, 2024

Choose a reason for hiding this comment

nscarcella Feb 23, 2024

Choose a reason for hiding this comment

turansky Feb 28, 2024 • edited

Choose a reason for hiding this comment

nscarcella Feb 23, 2024

Choose a reason for hiding this comment

nscarcella commented Sep 29, 2023 •

edited

nscarcella commented Sep 29, 2023 •

edited

nscarcella commented Sep 29, 2023 •

edited

nscarcella commented Sep 30, 2023 •

edited

saschanaz commented Sep 30, 2023 •

edited

saschanaz commented Dec 17, 2023 •

edited

saschanaz commented Dec 17, 2023 •

edited

saschanaz Feb 22, 2024 •

edited

turansky Feb 22, 2024 •

edited

turansky Feb 28, 2024 •

edited