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

ShallowRenderer.simulate supports batched updates #342

Merged
merged 2 commits into from May 16, 2016

Conversation

Projects
None yet
6 participants
@koba04
Copy link
Contributor

commented Apr 22, 2016

This PR is for ShallowWrapper.simulate() supports batched updates using ReactUpdates.batchedUpdates.

React.addons.TestUtils.Simulate.{eventName} is supporting batched updates.
ref. https://jsfiddle.net/koba04/6Lchbest/

ReactUpdates.batchedUpdates is exported as ReactDOM. unstable_batchedUpdates from React v0.14. In React v0.13, it is exported as React.addons.batchedUpdates.

It might be better I use the exported interface.
But I guess it makes your configurations complex than using ReactUpdates.batchedUpdates directly.

FYI: This batched updates feature is required #318.

Thanks.

@koba04 koba04 changed the title ShallowRenderer supports batched updates ShallowRenderer.simulate supports batched updates Apr 22, 2016

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2016

Interesting. So you are planning on using this to ensure that the batching behavior of setState is accurately depicted in #318?

@koba04

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2016

@lelandrichardson

So you are planning on using this to ensure that the batching behavior of setState is accurately depicted in #318?

Yes, it would be nice if I could ensure that.
But I'm not sure that it is possible because I'm trying it yet.

This is relevant to #318 but independent.

@@ -134,6 +134,8 @@ if (REACT013) {
};
}

const batchedUpdates = require('react/lib/ReactUpdates').batchedUpdates;

This comment has been minimized.

Copy link
@blainekasten

blainekasten May 10, 2016

Contributor

This might become an issue due to facebook/react#6460. depending on which way they go. Hopefully this might help sway them away.

This comment has been minimized.

Copy link
@aweary

aweary May 10, 2016

Collaborator

I think we'll likely have to be reactive when it comes to facebook/react#6460. We should definitely try to work with the React team if they want to support libraries like enzyme in some capacity.

Also @koba04 can you put this require up at the top with the main const React = require('react') statement, since this is not a version dependant import at all.

This comment has been minimized.

Copy link
@koba04

koba04 May 10, 2016

Author Contributor

@aweary I've updated it!

@koba04 koba04 force-pushed the koba04:simulate-supports-batched-updates branch from 270a544 to 066aefd May 10, 2016

@aweary

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

Since there is a public API for batchedUpdates maybe we should just wrap the batchedUpdates assignment call in the REACT013 check and use the appropriate access points mentioned in the OP. It's more complicated but at least we can avoid reaching into React/lib for now.

What do you think @blainekasten @lelandrichardson @koba04?

@koba04

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

@aweary I've updated this PR to use public APIs for batchedUpdates! It's not complicated for me 😄

@aweary

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

LGTM

1 similar comment
@nfcampos

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

LGTM

@nfcampos

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

I'll have to find out whether react batches updates for all handlers triggered by an event for #368, if anyone knows let me know

@aweary

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

@nfcampos a cursory glance suggest that they do but we'll have to investigate that further to verify.

@aweary

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

@ljharb @lelandrichardson it doesn't seem like the LGTM check seems to be working?

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

LGTM

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2016

@aweary it looks like there's still some configuration we need to do with it to add you guys. i'll see if @ljharb can finish that today. thanks!

@ljharb

This comment has been minimized.

Copy link
Member

commented May 10, 2016

@aweary turns out that the issue is lgtmco/lgtm#19 - i'll update our MAINTAINERS file in the meantime.

@koba04 koba04 force-pushed the koba04:simulate-supports-batched-updates branch from dbecc1b to 1c866e1 May 11, 2016

@koba04

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2016

rebased

@aweary

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2016

@koba04 can you rebase please? I think we can merge this afterwards.

@koba04 koba04 force-pushed the koba04:simulate-supports-batched-updates branch from 1c866e1 to bd5249c May 16, 2016

@koba04

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2016

@aweary rebased!

@aweary

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2016

Thanks @koba04!

@aweary aweary merged commit 49f30b2 into airbnb:master May 16, 2016

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@koba04

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2016

Thanks for your help 🎉

@koba04 koba04 deleted the koba04:simulate-supports-batched-updates branch May 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.