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

Generic DOM Events #1624

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

nscarcella
Copy link

@nscarcella nscarcella commented Sep 29, 2023

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
Copy link
Contributor

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
Copy link
Author

nscarcella commented Sep 29, 2023

@microsoft-github-policy-service agree

@HolgerJeromin
Copy link
Contributor

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
Copy link
Author

nscarcella commented Sep 29, 2023

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
Copy link
Contributor

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
Copy link
Author

nscarcella commented Sep 30, 2023

@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
Copy link
Contributor

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
Copy link
Contributor

Which command are you using btw?

@nscarcella
Copy link
Author

@saschanaz

Which command are you using btw?

tsc --extendedDiagnostics

@saschanaz
Copy link
Contributor

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

@nscarcella
Copy link
Author

@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
Copy link
Contributor

saschanaz commented Sep 30, 2023

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

@nscarcella
Copy link
Author

@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
Copy link
Contributor

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

@nscarcella
Copy link
Author

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
Copy link
Author

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

@DaSchTour
Copy link

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
Copy link
Author

@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
Copy link

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
Copy link
Contributor

saschanaz commented Dec 17, 2023

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:

  1. 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.
  2. extends Event<T> should rather be autogenerated rather than manually overridden by overridingTypes.jsonc.

@saschanaz
Copy link
Contributor

saschanaz commented Dec 17, 2023

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
Copy link
Author

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
Copy link
Contributor

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:

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

@nscarcella
Copy link
Author

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?

Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

export const genericEventSupertypes = new Set(["UIEvent", "MouseEvent"]);
Copy link
Contributor

@saschanaz saschanaz Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@turansky turansky Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@turansky turansky Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Remove T type parameter from ProgressEvent
    • To replace it later with common C
  2. D type parameter for CustomEvent and MessageEvent
    • T reserved as future type parameter for target (will work fine in handlers like FileReader.onload)
  3. 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 nscarcella marked this pull request as draft February 23, 2024 21:05
@@ -1186,6 +1207,8 @@ export function emitWebIdl(

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Request to change currentTarget in Event interface for lib.d.ts
6 participants