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

Use the correct SyntheticEvent for TestUtils.Simulate #6407

Closed
wants to merge 1 commit into from

Conversation

zpao
Copy link
Member

@zpao zpao commented Apr 4, 2016

This is currently an RFC as a different approach to #6380, doing what I proposed in a comment - to use the correct synthetic event for the given event. This ensures that the correct properties from the native event are copied and we won't trigger that warning. This is better than what I did in 6380 in that this will leave runtime and testing more inline and triggering the same warnings for setting extra properties after the event is created, as opposed to silencing those warnings blindly.

cc @spicyj

TODO:

  • Update the other event plugins so the API is consistent and all of the tests can actually pass

@sophiebits
Copy link
Collaborator

It seems like maybe it's useful to be able to provide extra properties – does this prevent that? I also didn't actually look at the code here yet and it looks like a lot to put in 15.0.

@zpao
Copy link
Member Author

zpao commented Apr 5, 2016

It seems like maybe it's useful to be able to provide extra properties – does this prevent that?

Yes, but it would mirror the actual behavior of Synthetic Events in non-tests. The extra properties are all copied the native event and then specific properties are copied over based on the event type (eg click is a mouse event and will copy over clientX). Anything else would be available on syntheticEvent.nativeEvent like usual.

it looks like a lot to put in 15.0

Yea, it's a bit more than I would want this late in the game but it's entirely tests, and it fixes the issue without resulting in different warnings in tests vs app. With no changes we'll have extra warnings in tests. With my other fix we'd be missing warnings in tests that show up in app code. We could probably just leave it either way and then take this in 15.1 (seems like a safe change there since it would only change tests), we'd just have to be aware of the difference.

FWIW, the changes to actual code are essentially no-ops (extra entry in the stack for error cases but otherwise it's an internal API addition).

@aweary
Copy link
Contributor

aweary commented Jan 26, 2017

@spicyj @zpao is this still something we want to consider getting in?

@nhunzaker
Copy link
Contributor

No update since the end of January. Is this something we want to revisit, or close?

@gaearon
Copy link
Collaborator

gaearon commented Nov 12, 2017

I'd feel fine with closing if there is an issue tracking it.

@gaearon
Copy link
Collaborator

gaearon commented Nov 21, 2017

Going to close as stale.

@gaearon gaearon closed this Nov 21, 2017
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.

7 participants