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

[Shallow]Call update() automatically through get querys #1499

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Jan 27, 2018

In Enzyme v3, I saw many issues about changes that we have to call update() after instance.setState().

Actually, wrapper.update() doesn't rerender even though the name implies to update something.
wrapper.update() only does to get a latest ReactElementTree from ReactShallowRenderer.
I think it would be nice if developers don't care about calling wrapper.update().

In this case, enzyme can't handle all cases that need to call wrapper.update().

expect(wrapper.find('p').text()).to.equal('0');
wrapper.find('.button').prop('onClick'); // Update the value to '1'
// enzyme should call wrapper.update() but enzyme can't handle that
// because the above calls ReactComponent instance's setState directly 
expect(wrapper.find('p').text()).to.equal('0');
wrapper.update(); // Get the latest nodes from ShallowRenderer
expect(wrapper.find('p').text()).to.equal('1');

In order to resolve this, this PR adopts a pull-based approach, which calls wrapper.update() in getter for internal nodes.
So we can get the latest node through the enzyme APIs (find, text, ...).

expect(wrapper.find('p').text()).to.equal('0');
wrapper.find('.button').prop('onClick'); // Update the value to '1'
// wrapper has nodes that are before updating.
expect(wrapper.find('p')/* Get the latest nodes from ShallowRenderer */.text()).to.equal('1');

This PR is for ShallowWrapper, but I could apply this to ReactWrapper as well.

But there is a limitation.
This PR doesn't cover that a wrapper isn't RootShallowWrapper.

let child = wrapper.find('p');
expect(child.text()).to.equal('0');
child.prop('onClick'); // Update the value to '1'
expect(child).to.equal('0');
// we have to get the child from the wrapper again
let child = wrapper.find('p');
expect(child).to.equal('1');

Covering the case might be difficult because enzyme has to get child from the updated RootShallowWrapper with the same condition.
But enzyme has to care about a case that can't get child due to the update.
I think it seems to be good that wrappers that are not RootShallowWrapper treat as immutable.

BTW. ShallowWrapper has the condition(this[ROOT] === this) in many methods, I think it becomes clearer if ShallowWrapper can be separated RootShallowWrapper and ChildShallowWrapper.

@@ -152,11 +152,17 @@ class ShallowWrapper {
return this[ROOT];
}

getRootNodeInternal() {
Copy link
Member

Choose a reason for hiding this comment

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

we tried pretty hard in v3 to minimize the public API. Can this be a normal function instead of a class method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed it.

@ljharb ljharb merged commit b05147c into enzymejs:master Jun 26, 2018
@koba04
Copy link
Contributor Author

koba04 commented Jun 26, 2018

Thanks!

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.

2 participants