Skip to content

Conversation

@saschanaz
Copy link
Contributor

@sandersn, how about this?

@sandersn
Copy link
Member

sandersn commented Jun 5, 2019

Is this useful anywhere else? I expected that lots of other event types would want to have custom targets?

@saschanaz
Copy link
Contributor Author

Might be better to start rather than doing all in once. IMO this will at least prevent a future breaking change "Where is my FileReaderProgressEvent" as we already saw one.

@sandersn
Copy link
Member

sandersn commented Jun 6, 2019

If this is a start that will have some follow-up, I want to understand what the follow-up will be. Are there other subtypes of ProgressEvent that will provide different arguments to T?

If the only subtype of ProgressEvent is FileReader, then I think it's too expensive and complex to create a generic type for just one case.

@saschanaz
Copy link
Contributor Author

Are there other subtypes of ProgressEvent

Current users of ProgressEvent in lib.d.ts include ApplicationCache, Document, GlobalEventHandlers, and XMLHttpRequest,

@sandersn
Copy link
Member

sandersn commented Jun 7, 2019

How would ApplicationCache customise ProgressEvent.target? Would T=ApplicationCache ?

@saschanaz
Copy link
Contributor Author

saschanaz commented Jun 7, 2019

Yep, and T=XMLHttpRequest.

This can be generalized for nearly all Event sub-interfaces, so we even could do some automation in the future rather than manual generic field. (...while generics are known to be heavy.)

@sandersn
Copy link
Member

sandersn commented Jun 7, 2019

OK, that makes sense. Can you make the change for ApplicationCache, Document, GlobalEventHandlers, and XMLHttpRequest in this PR?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

One naming request and one question.

@sandersn sandersn merged commit a20c418 into microsoft:master Jun 12, 2019
@saschanaz saschanaz deleted the progressevent-t branch June 14, 2019 03:20
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.

2 participants