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

Fix Event.currentTarget type #207

Closed
wants to merge 12 commits into from
Closed

Fix Event.currentTarget type #207

wants to merge 12 commits into from

Conversation

zhengbli
Copy link
Contributor

@zhengbli zhengbli commented Mar 1, 2017

This should fix microsoft/TypeScript#299

@zhengbli zhengbli changed the title WIP Fix events Fix Event.currentTarget type Mar 1, 2017
@zhengbli
Copy link
Contributor Author

zhengbli commented Mar 1, 2017

Travis seems to have problems downloading mono bits. Will retry CI later.

@@ -399,9 +399,9 @@ declare var Event: {
}

interface EventTarget {
addEventListener(type: string, listener?: EventListenerOrEventListenerObject, useCapture?: boolean): void;
addEventListener(type: string, listener?: EventListenerOrEventListenerObject<this>, useCapture?: boolean): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

in general we do not want to set this as it turns all the types that inherit from EventTarget (almost all DOM types) as generic, and every time you reference them we make an instantiation, more memory and more time spent checking the default lib file.

can we make EventTarget and pass the current type through?

@zhengbli
Copy link
Contributor Author

@mhegazy now there are no <this> as type parameters any more. Further comments?

@DaSchTour
Copy link

Why is this still pending? What is the next step? How could I maybe help?

@saschanaz
Copy link
Contributor

This PR still has performance problem:

Files:             2
Lines:         19211
Nodes:         85546
Identifiers:   30004
Symbols:       96920
Types:         25399
Memory used: 101439K
I/O read:      0.00s
I/O write:     0.00s
Parse time:    0.36s
Bind time:     0.17s
Check time:    1.12s
Emit time:     0.02s
Total time:    1.67s

Compared to the current master:

Files:            2
Lines:        19316
Nodes:        84450
Identifiers:  29164
Symbols:      22004
Types:         5790
Memory used: 56757K
I/O read:     0.03s
I/O write:    0.00s
Parse time:   0.38s
Bind time:    0.16s
Check time:   0.64s
Emit time:    0.02s
Total time:   1.20s

@DaSchTour
Copy link

Any progress on this? It's really annoying to have as as HTMLElement all over my code where I use EventListeners. That really messes up my code and every second week somebody asks me why "Events are broken" in TypeScript or I discover code where Events are "casted" to any just to get around this wrong type definition.

@saschanaz
Copy link
Contributor

Generics on TS is fairly expensive, probably the performance problem should be somehow managed before any fix for this issue.

@DaSchTour
Copy link

Why is this closed? This is still an issue! And explanation on why this is closed would be very helpful.

@saschanaz
Copy link
Contributor

saschanaz commented Apr 4, 2019

I guess because the author stopped working on TS. (And this PR is so old that it is simply incompatible with the current code.)

@sandersn
Copy link
Member

sandersn commented Apr 4, 2019

The PR is two years out of date, back when (1) the code was written in F# (2) the IDL source was IE or Edge.

microsoft/TypeScript#299, the issue tracking this problem, is still open and has 31 upvotes right now.

@DaSchTour
Copy link

Okay. Interesting news. I wonder why this isn't communicated more offensively. I guess many people would like to know more about such changes. It would be nice to see such changes and information in TypeScript Release Notes and Roadmap.

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
7 participants