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

Removing or documenting the need for Shallow Renderer's .update() #360

Closed
jwbay opened this issue May 2, 2016 · 5 comments · Fixed by #490
Closed

Removing or documenting the need for Shallow Renderer's .update() #360

jwbay opened this issue May 2, 2016 · 5 comments · Fixed by #490

Comments

@jwbay
Copy link
Contributor

jwbay commented May 2, 2016

The problem

When testing interactions with shallow rendered components, what interactions do and don't need .update() after them can feel inconsistent. If the component's rendered output doesn't match what you think it should after performing some action, you can end up littering tests with .update() hoping to get lucky.

It wasn't until I peeked at the source and saw the .update() call in .simulate() that everything made sense. Even knowing that unless I'm using a few "blessed" methods I'll need to call .update() myself it's still going to trip me up at times.

The solution

Is it feasible to wrap rendering such that whenever a component is rendered .update() is called afterward automagically? Via wrapping .prototype.render, I suppose, though hopefully there's a more clever solution or hook somewhere.

Or at least, an update to .update()'s docs to be clear about the cases where it's called for you could help greatly. From what I saw, this is just .simulate and .setProps/State/Context?

@nfcampos
Copy link
Collaborator

One thing we could try to do that would help with some of these situations is make sure every time the component internally calls this.setState we call forceUpdate afterwards. But I'd say this is only worth it if we can find similar fixes for the other situations in which a shallow-rendered component might get out of date, is this the only one?

@jwbay
Copy link
Contributor Author

jwbay commented May 13, 2016

wrapping/injecting around this.setState would probably also cover the 99% case. There are two cases I keep hitting, and automagically calling update after this.render or this.setState should handle both.

Example test for each: master...jwbay:update

The impure render case currently in the example for .update's documentation wouldn't benefit, but hopefully that's a serious edge case 😅

@nfcampos
Copy link
Collaborator

nfcampos commented May 13, 2016

I need to think about this some more, I wonder if there's some reason why the shallow renderer from react TestUtils doesn't do this "update after all calls to setState"

(as for impure render methods there's nothing we can do, those will always have to be manual)

@guncha
Copy link

guncha commented Jul 6, 2016

TestUtils does update the rendered output. The problem is that Enzyme doesn't know anything about it and happily keeps using the old output.

It needs to call renderer.getRenderOutput() which is why I recommend making this.node on ShallowWrapper on the root wrapper an accessor that calls the getRenderOutput function. It's a cheap function (1 function call, 1 if statement, ~5x property access) so performance wouldn't be an issue.

This way, the rendered output will always be up to date regardless of what goes on behind the scenes.

jwbay added a commit to jwbay/enzyme that referenced this issue Jul 8, 2016
…tput is always fresh. removes need for .update, fixes enzymejs#360
jwbay added a commit to jwbay/enzyme that referenced this issue Jul 8, 2016
…tput is always fresh. removes need for .update, fixes enzymejs#360
jwbay added a commit to jwbay/enzyme that referenced this issue Jul 8, 2016
…tput is always fresh. removes need for .update, fixes enzymejs#360
@hptk
Copy link

hptk commented Jul 28, 2016

I had a related issue in #517 and would like to get behind OP on this.

As a relatively new user to both React and Enzyme, it feels very natural to me that calling setState() on the wrapper would trigger an update, making the test environment as much as possible reassemble the browser DOM where updating state triggers a re-render automatically.

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

Successfully merging a pull request may close this issue.

4 participants