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

[New] shallow: add .dive() #618

Merged
merged 3 commits into from
Oct 3, 2016
Merged

[New] shallow: add .dive() #618

merged 3 commits into from
Oct 3, 2016

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Sep 30, 2016

The goal of this method is to make testing of HOCs (higher-order components) easier - withStyles, flux-store-connected components, etc.

Typically, the workarounds are either:

  • wrapper.find('StringNameOfComponent').shallow(): brittle, because it's using strings (strings are bad); also annoying to type out.
  • wrapper.childAt(0).shallow(): doesn't check if there's more than one child, and is annoying to type out.

In addition, if suddenly there's a DOM element in there, you might suddenly start shallow-rendering it without knowing, and your tests might erroneously be passing.

This method allows wrapper.dive() - ie, it requires there be one non-DOM child, and shallow-renders it for you.

If you need multiples, you chain them - wrapper.dive().dive() - for now we have intentionally omitted the ability to dive multiple levels at once, so that tight coupling to HTML structure is discouraged.

Longer term, we have a much better solution in mind for HOCs, but this is still useful in the interim.

@lelandrichardson
Copy link
Collaborator

LGTM!

@lelandrichardson
Copy link
Collaborator

(also, I like "dive"!)

Copy link
Collaborator

@lelandrichardson lelandrichardson left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -207,3 +207,6 @@ Returns whether or not all of the nodes in the wrapper match the provided select

#### [`.everyWhere(predicate) => Boolean`](ShallowWrapper/everyWhere.md)
Returns whether or not all of the nodes in the wrapper pass the provided predicate function.

#### [`.dive([options]) => ShallowWrapper`](ShallowWrapper/dive.md)
Throws unless the wrapper contains only one non-DOM Component child; otherwise, returns a shallow wrapper around it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we are focusing too much on the condition in which it throws. The fact that it throws should just be a note.

The description should be something like "shallow render the one child of the current wrapper, and return a wrapper around the result"

@@ -0,0 +1,52 @@
# `.dive([options]) => ShallowWrapper`

Throws unless the wrapper contains only one non-DOM Component child; otherwise, returns a shallow wrapper around it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.

Copy link
Collaborator

@aweary aweary left a comment

Choose a reason for hiding this comment

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

Can we add some tests as well?

@ljharb
Copy link
Member Author

ljharb commented Oct 2, 2016

@aweary doh! thanks, will add.

@ljharb ljharb force-pushed the dive branch 2 times, most recently from d52115b to 3dd8111 Compare October 2, 2016 06:08
@ljharb
Copy link
Member Author

ljharb commented Oct 2, 2016

@aweary added :-)

@ljharb ljharb force-pushed the dive branch 2 times, most recently from 15cd707 to c2414ca Compare October 2, 2016 22:19
@ljharb ljharb merged commit a26f5a4 into master Oct 3, 2016
@ljharb ljharb deleted the dive branch October 3, 2016 04:51
const wrapper = shallow(<Foo />);
expect(wrapper.find('.in-bar')).to.have.length(0);
expect(wrapper.find(Bar)).to.have.length(1);
expect(wrapper.dive().find('.in-bar')).to.have.length(1);

Choose a reason for hiding this comment

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

I suspect this would fail because wrapper wraps a DOM element and by design, dive (currently?) throws in that case.

Don't you mean expect(wrapper.children().dive().find('.in-bar')).to.have.length(1); instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, thanks, I did mean that :-) fixed in e722625

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