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

koba04
Copy link
Contributor

@koba04 koba04 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
Copy link
Collaborator

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

@koba04
Copy link
Contributor Author

koba04 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aweary I've updated it!

@koba04 koba04 force-pushed the simulate-supports-batched-updates branch from 270a544 to 066aefd Compare May 10, 2016 14:53
@aweary
Copy link
Collaborator

aweary 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
Copy link
Contributor Author

koba04 commented May 10, 2016

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

@aweary
Copy link
Collaborator

aweary commented May 10, 2016

LGTM

1 similar comment
@nfcampos
Copy link
Collaborator

LGTM

@nfcampos
Copy link
Collaborator

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
Copy link
Collaborator

aweary commented May 10, 2016

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

@aweary
Copy link
Collaborator

aweary commented May 10, 2016

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

@lelandrichardson
Copy link
Collaborator

LGTM

@lelandrichardson
Copy link
Collaborator

@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
Copy link
Member

ljharb 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 simulate-supports-batched-updates branch from dbecc1b to 1c866e1 Compare May 11, 2016 00:27
@koba04
Copy link
Contributor Author

koba04 commented May 11, 2016

rebased

@aweary
Copy link
Collaborator

aweary commented May 14, 2016

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

@koba04 koba04 force-pushed the simulate-supports-batched-updates branch from 1c866e1 to bd5249c Compare May 16, 2016 00:40
@koba04
Copy link
Contributor Author

koba04 commented May 16, 2016

@aweary rebased!

@aweary
Copy link
Collaborator

aweary commented May 16, 2016

Thanks @koba04!

@aweary aweary merged commit 49f30b2 into enzymejs:master May 16, 2016
@koba04
Copy link
Contributor Author

koba04 commented May 16, 2016

Thanks for your help 🎉

@koba04 koba04 deleted the simulate-supports-batched-updates branch May 16, 2016 02:30
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.

6 participants