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

React adapter 16: portals and roots may render fragments #1710

Merged
merged 2 commits into from
Jul 27, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jul 9, 2018

This matches what react-test-renderer does as well.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this seems great!

</React.Fragment>
);

expect(wrapper).to.have.lengthOf(2);
Copy link
Member

Choose a reason for hiding this comment

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

can we add assertions that the wrapper has a p and a span?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've rebased this, but the tests are failing, so it still needs some work.

I also added a commit that you can feel free to remove, it was a shot in the dark.

@jquense
Copy link
Contributor Author

jquense commented Jul 24, 2018

I removed the two shallow tests, i'm not sure why i added them in the first place. ShallowRenderer doesn't support mounting a Fragment, and a Portal doesn't make any sense, since it's a DOM specific element.

const isStateful = Component.prototype && (
Component.prototype.isReactComponent
|| Array.isArray(Component.__reactAutoBindPairs) // fallback for createClass components
);
if (!isStateful) {

if (!isStateful && typeof Component === 'function') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just so we get a clearer error is a user tries to mount a Fragment or Portal, they will see the ShallowTestRenderer informative error instead of the much less clear "Component is not a function"

@ljharb
Copy link
Member

ljharb commented Jul 25, 2018

@jquense I'd actually added them, for test parity. We should be able to shallow-render a fragment and a Portal.

@jquense
Copy link
Contributor Author

jquense commented Jul 25, 2018

I'm not sure what you want to do in this case, it doesn't make much sense to me to intentionally hobble the mount cases for parities sake in my mind.

The main issue tho is that upstream shallow renderer does not allow it. Same is true for the new context types as well

@ljharb
Copy link
Member

ljharb commented Jul 26, 2018

Hmm. I certainly don't want to limit what mount can do.

However, I also don't want to limit what shallow can do just because the upstream renderer can't handle it. Is there any way that a Portal, Fragment, or Context component couldn't just be treated like a div, in that in shallow mode it simply renders its children?

@jquense
Copy link
Contributor Author

jquense commented Jul 26, 2018

I think we can probably get clever and hack something together but maybe we can split that into a new PR? The goal here was really to support selectors through these types deep in element tree not at the root. at moment the adapter assumes they can only render single children. The tests have sort of accidentally brought to light this inconsistency, but it's one that already exists, not one introduced by this PR

@ljharb ljharb force-pushed the fix-fragment-support branch from 8fd62dd to f0e8ae5 Compare July 27, 2018 21:26
@ljharb ljharb force-pushed the fix-fragment-support branch from f0e8ae5 to 30b7970 Compare July 27, 2018 22:13
@ljharb ljharb merged commit 30b7970 into enzymejs:master Jul 27, 2018
@ljharb
Copy link
Member

ljharb commented Jul 27, 2018

Thanks! Let's do those changes in a followup as you suggested.

@ljharb ljharb mentioned this pull request Aug 11, 2018
41 tasks
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