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

Fixes shallow rendering of component children (fixes #5292) #5299

Closed
wants to merge 1 commit into from
Closed

Fixes shallow rendering of component children (fixes #5292) #5299

wants to merge 1 commit into from

Conversation

iredelmeier
Copy link

This should fix the issues with shallow rendering and _owner mentioned in #5292. Assertions around the equality of component children expect _owner to be null, but shallow rendering was setting a real _owner.

The test case I provided is based on the failure mentioned in #5292. Setting the component body directly still worked with the original NoopInternalComponent, and using simpler children didn't require the full fix - that's why the component is kind of unusual :)

I've seen similar issues when composing (functional) components, but there's a secondary issue there around the component's type that I can submit a separate issue and PR for.

@gaearon
Copy link
Collaborator

gaearon commented Mar 27, 2016

Thank you for submitting the PR!

The downside of this is that the rendered element no longer uses the children we pass to it directly. This may break other people’s tests that rely on referential identity, e.g. when people do something like expect(result.props.children).toBe(element.props.children) (rather than toEqual). I’m not sure which pattern is more common.

@gaearon
Copy link
Collaborator

gaearon commented Mar 27, 2016

An alternative solution would be to hide _owner from being enumerable. I submitted a PR that does this in #6355. If there are no issues with this approach, I think it would be a simpler fix that wouldn’t affect other scenarios negatively. Thank you for getting this rolling!

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2016

I fixed this in a slightly different way in #6362 so that we avoid having _owner in the test environment in the first place. Thank you very much for taking time to contribute!

@gaearon gaearon closed this Apr 14, 2016
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.

4 participants