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

[TestUtils] Implement type property in Simulate.<eventName> #6154

Merged
merged 1 commit into from
Jun 26, 2016

Conversation

quantizor
Copy link
Contributor

Although it is unreasonable to set every possible property for simulated events, type is useful for shared event handlers that have different behaviors.

Closes #6100

Although it is unreasonable to set every possible property for
simulated events, `type` is useful for event handlers that are shared
between types and potentially have different behaviors.
@gaearon
Copy link
Collaborator

gaearon commented Mar 28, 2016

Thanks for contributing! I’m closing because I think this should already be possible for any property, so there is no need to add a special support for type: #6100 (comment). Please let me know if I’m wrong!

Cheers.

@gaearon gaearon closed this Mar 28, 2016
@quantizor
Copy link
Contributor Author

quantizor commented Mar 29, 2016 via email

@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2016

Yeah perhaps you’re right. Since we’re setting target anyway, might as well set the type.
I don’t feel strongly either way. I wonder why this hasn’t come up before though.

Should this line have the same addition too for consistency? I don’t know this code well so can’t say.

I’ll defer to @zpao on whether we want this.

@gaearon gaearon reopened this Mar 29, 2016
@quantizor
Copy link
Contributor Author

@gaearon The tests don't break without that other line, so ¯\_(ツ)_/¯

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2016

Yeah but you’re not using SimulateNative, are you? I’m not sure who’s using it because it’s undocumented but quite extensive. Maybe Facebook uses it internally; I haven’t checked.

@quantizor
Copy link
Contributor Author

@gaearon Yes that's true, I've only ever used Simulate in my testing.

@quantizor
Copy link
Contributor Author

@gaearon anything else you want me to do here?

@gaearon gaearon merged commit 5a20d44 into facebook:master Jun 26, 2016
@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

Let’s get it in, don’t see any harm in this. 👍

@zpao zpao modified the milestones: 15-next, 15.3.0 Jul 7, 2016
zpao pushed a commit that referenced this pull request Jul 13, 2016
Although it is unreasonable to set every possible property for
simulated events, `type` is useful for event handlers that are shared
between types and potentially have different behaviors.
(cherry picked from commit 5a20d44)
@uxtx
Copy link

uxtx commented Aug 3, 2016

Hi all, I've encountered a few errors in a few external apps (react-select is a public-facing one) due to mutating the event type with the toLowerCase method. Replacing this with toString() appears to resolve the issue - I've created a pr for this. #7419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants