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

more precise type annotation for WildCardEventHandler #58

Merged
merged 3 commits into from
Jul 5, 2017

Conversation

tungv
Copy link
Contributor

@tungv tungv commented Jun 14, 2017

an attempt to get away with the hack in type annotation from the previous fix

@developit
Copy link
Owner

I might have to ask @aweary for help reviewing 😇

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

It's cool that flow supports typing based on literal values. I think this is 👍, pending my comment.

src/index.js Outdated
// A map of event types and their corresponding event handlers.
type EventHandlerMap = {
'*': WildCardEventHandlerList,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be:

'*'?: WildCardEventHandlerList

Otherwise, it means that EventHandlerMap should always have a "*" key

@tungv
Copy link
Contributor Author

tungv commented Jun 15, 2017

hi @aweary, I addressed your comment. Thank you!

@tungv
Copy link
Contributor Author

tungv commented Jun 20, 2017

@developit @aweary is this good to merge?

@Andarist
Copy link
Contributor

Andarist commented Jul 5, 2017

any progress on this? would be great to have this fixed, maybe @tunnckoCore can handle this?

@tunnckoCore
Copy link
Collaborator

Hey, thanks for the ping. I'll try to review it soon :)

@tunnckoCore
Copy link
Collaborator

Seems okey, probably... but i'm not so TS/Flow guy like @developit ;d

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

FWIW it looks good to me :shipit:

@developit developit merged commit 4517958 into developit:master Jul 5, 2017
@Andarist
Copy link
Contributor

Andarist commented Jul 6, 2017

@developit one last thing - could this be released to npm? :)

@tungv tungv deleted the patch-1 branch July 9, 2017 14:44
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.

None yet

5 participants