Parse props in instMatchesObjectProps/instHasProperty for all instances #377

aweary · 2016-05-11T02:19:42Z

Resolves #376

The current implementation just returns false if isDOMComponent(inst) is false. We can still check props on those instances by just passing inst directly to propsOfNode.

lelandrichardson · 2016-05-11T04:01:44Z

Wow, quick turnaround! 👍

This definitely fixes what I would consider a bug. We do have to be careful though, because this could cause some unexpected failured by some people. For instance:

Let's say I have a component:

function FancyDiv(props) {
  return <div data-fancy {...props} />
}

Then, If I render some component:

function Foo() {
  return (
    <FancyDiv className="foo" />
  );
}
const wrapper = mount(<Foo />);

At this point, am I right in assuming that prior to this change, wrapper.find('.foo') would return a wrapper of length 1, but after the change it would return a wrapper of length 2?

I was kind of under the impression that it already worked this way... but if it didn't, this may blow up in some peoples' faces and cause unexpected test failures on update.

lelandrichardson · 2016-05-11T04:58:24Z

I guess my example technically isn't true, since the branches between instHasClassName and instHasObjectProperty are different.... BUT, it does seem like:

wrapper.find({ className: 'foo' }) should be essentially equivalent to wrapper.find('.foo') (without the class list matching semantics considered.

Similarly: wrapper.find('#id') ~= wrapper.find({ id: 'id' }), but at this point that wouldn't be true.

I might be okay with that considering the css selector maybe should sort of "opt in" to looking at only DOM nodes... but I'm really not sure here.

Additional note. WIth this change, it seems like: wrapper.find([className="foo"]) and wrapper.find('.foo') will also not always find the same nodes. (even without considering class list semantics).

blainekasten · 2016-05-11T05:52:40Z

Agreed! Awesome turn around!

I might be okay with that considering the css selector maybe should sort of "opt in" to looking at only DOM nodes... but I'm really not sure here.

I think this is a classic argument of correctness vs intuitiveness. Most developers using enzyme aren't going to look at docs until they absolutely need to. They'll just do what they assume should work. Thus the argument is that selectors should be intuitive.

I would think we should maintain interactions like:
.foo === {className: 'foo'} === '[className="foo"]'

@lelandrichardson are you sure your thoughts are fully right? I would assume some internal tests would break if that was the case... If not, In a different PR, we should have some simple tests to make sure the selectors all match up.

nfcampos · 2016-05-11T15:19:16Z

This PR breaks a little bit of parity between shallow and mount, ie.

const Root = ({ classProp }) => (
        <div className={classProp}>
          <MockLink to="/">First</MockLink>
          <MockLink to="/second">Second</MockLink>
        </div>
      );

      const wrapper = mount(
        <Root classProp="yay" />
      );

      const wrapperr = shallow(
        <Root classProp="yay" />
      );

      expect(wrapper.find({ classProp: 'yay' }).props()).to.eql(
        wrapperr.find({ classProp: 'yay' }).props()
      )
      expect(wrapper.find({ className: 'yay' }).props()).to.eql(
        wrapperr.find({ className: 'yay' }).props()
      )

wrapperr.find({ classProp: 'yay' }) doesn't find any nodes

aweary · 2016-05-11T15:26:54Z

@nfcampos wrapperr being the shallow wrapper? This PR only changesMountedTraversal so is this another issue with ShallowWrapper? It seems the ReactWrapper behavior is correct while the ShallowWrapper behavior is not. I'll try and reproduce and investigate that.

nfcampos · 2016-05-11T15:31:30Z

yes wrapperr was the shallow wrapper. to reproduce you just have to run that test case in my comment above. I guess both MountedTraversal and ShallowTraversal suffered from this bug, this PR fixes it for MountedTraversal. For ShallowTraversal this bug only applies to the root node since that's the only one that in shallow has both a "DOM" representation and an instance

lelandrichardson · 2016-05-11T16:04:53Z

This isn't a bug with ShallowTraversal. This behavior is expected for shallow. Shallow only looks at a slice of the render tree, and the root node is not a part of that slice. This has always been true (though confusing to many).

I would assume some internal tests would break if that was the case...

This is the problem with putting too much trust into tests :)

I'm pretty confident in the behavior i've assumed above, but we should definitely write some tests to make sure. Due to (mostly my) laziness, a lot of tests overly focus on traversing DOM nodes and not real composite components. Better tests could (should) certainly be written.

aweary · 2016-05-11T16:10:24Z

So, to address the other broader issues. I think that

.foo === {className: 'foo'} === '[className="foo"]'

Is generally a good idea. I was surprised this change didn't break any existing tests. I'd like to keep the API easy to use and figure out how to make this a non-breaking change, but I think the current behavior is wrong. I think a big issue is that the test suite for .find() really only tests elements where isDOMComponent is true, so this bug has slipped through until now. We should make sure all find functionality works similarly for wrapping components like <MockLink/>.

edit: didn't see @lelandrichardson's comment above before posting that says essentially the same thing 👅

At this point, am I right in assuming that prior to this change, wrapper.find('.foo') would return a wrapper of length 1, but after the change it would return a wrapper of length 2?

As your second comment suggets, wrapper.find('.foo') does only return a length of 1 due to the branch using instHasClassName.

Using wrapper.find({className: 'foo'}) does return a wrapper with a length of 2. The issue I see is that technically it should return 2 if the className prop is being applied to two elements. I'm not sure there's a way for us to identify if a prop is being spread on a parent component just so it can be used by a child.

If anything find('.foo') should be implemented so that it also returns 2. I'm curious what you guys think. I think that ideally we should:

Fix instHasClassName (and all other predicates) so that it behaves the same way instMatchesObjectProps does
Add tests using components that fail the isDOMComponent check as well.

nfcampos · 2016-05-11T16:13:19Z

Shallow only looks at a slice of the render tree, and the root node is not a part of that slice.

@lelandrichardson I see. Then I guess this one shouldn't work...

shallow(<div className='hey'><span>abc</span></div>).find({className: 'hey'}).length

If the root node is not taken into account then that should return 0 instead of 1, no?

lelandrichardson · 2016-05-11T16:21:16Z

@nfcampos that test will return 1, but only because shallow treats passing in DOM elements to the root as a special case (for better or worse, i guess).

shallow(<div />)

at the moment is pretty much the same as doing this:

function Mock() { return <div /> }
shallow(<Mock />)

Under the hood it isn't actually doing this, but it's not really far off.

nfcampos · 2016-05-11T16:25:52Z

@lelandrichardson I see, makes sense, even if it is slightly surprising.

aweary · 2016-05-11T16:31:56Z

I think special casing DOM nodes over composite components is probably not ideal, and at least should be documented if it's not already, but I think that's a different issue 😃

lelandrichardson · 2016-05-11T16:42:40Z

@nfcampos @aweary if you want to really understand the reason, it can be stated like this:

Consider the full react render tree as a tree in 3-dimensional space
Consider the root component of a shallow call (ie, Foo in the case of wrapper(<Foo />)) as z=0 in this space
In the case of shallow, we choose to look only at z=1, which includes essentially the full output of render() of the <Foo /> component.
In the case of Foo being a DOM node, there is no z=1.

As a result, instead of making this an error case, we simply fall back to looking at z=0 instead of z=1.

I'm not sure if I consider this behavior wrong or less than ideal, but I do think that the fact that a large subset of our tests happen to test this case rather than the composite component case is the real problem.

aweary · 2016-05-11T17:01:11Z

@lelandrichardson maybe we should open an issue to discuss expanding the test suite to cover composite components as that seems to be a superset of an issue we're seeing with this PR specifically.

edit: #380

lelandrichardson · 2016-05-11T17:27:15Z

Here's where I'm at:

I'm pretty sure that we should have .find({ foo: "bar" }) match any node with the prop foo="bar", whether it's a leaf node or not. This is the bug that this PR fixes.
I'm not so sure that (in the case of mount) .find('.foo') should look at both DOM nodes and Composite Nodes for a className prop including "foo". People might assume that a '.foo' selector should find exactly what document.querySelectorAll(...) would find.
There's also a third case, where we want to know if .find('[foo="bar"]') should be the same as .find({ foo: "bar" }). I suspect the most sensible option here is to just do whatever we decide to do with (2).

This is only a problem with mount.

If we decide string-based selectors should look at composite nodes, it will work mostly as I originally intended for it to work, but will also likely break many existing tests.

If we decide they should not look at composite nodes, string selectors will work more closely to how querySelectorAll works, but will also deviate from the semantics of our object selector notation. This will also probably not break existing tests (or at least a lot fewer of them?)

aweary · 2016-05-11T17:42:08Z

I think that string-based selections should behave like querySelectorAll. Essentially find will only match composites if you're using the object selector.

Making string-based selections act like querySelectorAll makes it intuitive, but it also makes it inconsistent. I think as long as we document that then that's the better of the two options.

nfcampos · 2016-05-11T18:07:28Z

const Foo = ({className}) => <div className={className}>abc</div>
mount(<Foo className='yay' />).find({className: 'yay'}) // 2 nodes

That 2nd line returning 2 nodes is certainly never what anyone wants out of this API so that will always be surprising/frustrating. If we think it's worth it for that to return 2 so that you can find DOM nodes with find({...}) syntax then I guess it's worth the 'confusingness'. Otherwise, for me it makes some sense that string CSS-like selectors work only on DOM nodes and object selectors work only on composite nodes. (But I realise after having written this that object selectors before this PR worked only on DOM nodes, instead of only on composite nodes, so I guess that ship has sailed...)

lelandrichardson · 2016-05-11T18:18:01Z

That 2nd line returning 2 nodes is certainly never what anyone wants out of this API so that will always be surprising/frustrating.

In the case of { className: 'foo' } it's easy for this to feel true, but I don't think this is true in the general case (for props that don't have special meaning for the DOM... and in the case of className, using string-based selectors will certainly be more useful.

nfcampos · 2016-05-11T18:18:58Z

My point with the API not being surprising/confusing is to avoid eg. this:

const Foo = ({className}) => <div className={className}>abc</div>
const Bar = ({n}) => <div>
  {[...Array(n-1)].map(() => <Foo className='yay' />)}
</div>
mount(<Bar n={2} />).find({className: 'yay'}).length // even though it returns 2 like I expected I only rendered one...

aweary · 2016-05-11T18:36:52Z

@nfcampos I think looking at the output of debug shows why it's returning 2 and not 1.

<Bar n={2}>
  <div>
    <Foo className="yay">
      <div className="yay">
        abc
      </div>
    </Foo>
  </div>
</Bar>

If we special case string-based selectors then we'd expect find('.yay') to return 1 and find({className: 'yay'}) to return 2. Im voting that we do exactly that and parse composite props only with objects and then we document that fact. I can see both sides of the argument but we have to be able to query composite components with find and this seems like the lesser of two evils.

nfcampos · 2016-05-11T18:40:53Z

@aweary but since the test passed you'd never even look at the output of debug :)
anyway, it basically comes down to whether we think it's useful for find to return a wrapper that contains both composite and native nodes. because up until now it didn't do that, it was always one kind or the other. I just can't see any possible use for wanting a set of nodes of both types mixed in together, is there one?

aweary · 2016-05-11T18:47:32Z

I think the fact that it only returned props for DOM nodes is considered a bug

I'm pretty sure that we should have .find({ foo: "bar" }) match any node with the prop foo="bar", whether it's a leaf node or not. This is the bug that this PR fixes.

So just supporting composite components would just move the problem around.

I can understand supporting methods that only check DOM nodes as some people might want to just assert values on the DOM. I'm not entirely sure it'd be useful to have a method that only checks composite props, but I think one big issue with that is it would likely break a lot of existing tests. Maybe we could discuss a different approach to allowing composite-only prop queries if there are use cases we want to support?

Also, @lelandrichardson

There's also a third case, where we want to know if .find('[foo="bar"]') should be the same as .find({ foo: "bar" }). I suspect the most sensible option here is to just do whatever we decide to do with (2).

I agree with that. If they're using syntax that they'd be using with querySelectorAll then I think it should have similar behavior.

nfcampos · 2016-05-11T19:12:26Z

@lelandrichardson summing up my position on your 3 points

if this is considered a bug then yes let's make object selectors match both DOM and composite nodes (though as I said above I consider it a pretty weird api to match both types of nodes in the same find call)
and 3. IMO string selectors should work only for DOM nodes

aweary · 2016-06-09T13:44:24Z

@nfcampos @lelandrichardson @blainekasten

I updated this PR so that the object syntax will query composite and DOM nodes, which the CSS selector queries will only match DOM nodes (the existing behavior).

aweary · 2016-07-05T15:26:20Z

ping @nfcampos @lelandrichardson @blainekasten @ljharb

Any more feedback on this?

lelandrichardson · 2016-07-08T04:43:05Z

@aweary I'm still debating the right path on this. I'm going to try to get a minor release out tonight, and then the next release may be a breaking change with a couple that are queued up, and I might include this one in there (whether it's a breaking change or a bug fix is debatable, but it will definitely cause some tests to fail that currently pass... so we need to be conscious of that...)

MrLeebo · 2017-01-03T06:00:42Z

@aweary @lelandrichardson @nfcampos

TLDR: How about `wrapper.find(..., {includeNonDom: true})` and deprecate selectors targeting non-DOM components when mounted?

Now for the long version...

I can only bring a newbie's perspective to this problem, but as somebody who has recently spent hours trying to understand why my tests and queries were not returning what I expected, maybe a pair of fresh eyes can help?

I wrote a component to test with that just draws a number of divs based on an integer you pass in:

const RepeatDiv = ({depth}) => (<div>{depth > 1 && <RepeatDiv depth={depth-1} />}</div>)

// Example: <RepeatDiv depth={3} />
// Output:
// <div><div><div /></div></div>

Then I looked at how it get rendered in enzyme (v2.7.0)

console.log(shallow(<RepeatDiv depth={3} />).debug())
console.log(mount(<RepeatDiv depth={3} />).debug())

// shallow
<div>
<RepeatDiv depth={2} />
</div>

// mount
<RepeatDiv depth={3}>
  <div>
    <RepeatDiv depth={2}>
      <div>
        <RepeatDiv depth={1}>
          <div />
        </RepeatDiv>
      </div>
    </RepeatDiv>
  </div>
</RepeatDiv>

Even though mount is supposed to represent the DOM render, it still includes the non-DOM components. At first, I thought this was just a quirk with debug() trying to be helpful and that the selectors would return the DOM components only. However...

assert.equal(wrapper.find('RepeatDiv').length, 0) // fail. 3 != 0

Oops, that's not true. They show up in the results and could potentially impact my counts if I'm not careful. So I guess the mounted selectors do find non-DOM components...?

assert.equal(wrapper.find('RepeatDiv[depth=2]').length, 1) // fail. 0 != 1

Nope. Not true either. Turns out, only the display name selector finds them, as far as I can tell all other selectors ignore them. This behavior was inconsistent and quite a bit surprising and explained a lot of the trouble I had previously suffered trying to write my first tests (though I didn't realize the cause at that time).

I think that wrapper.find('RepeatDiv').length should return zero for mounted components, but since that would be a breaking change I think you should get a deprecation warning if you try to use the display name selector to find a non-DOM component and the behavior should change 2 releases hence.

At the same time I think you should be able to find non-DOM components on purpose by passing in an option like wrapper.find('RepeatDiv[depth=2]', {includeNonDom: true})

Does that make sense?

ljharb · 2018-08-07T07:28:15Z

The linked issue is closed, and seems to be fixed in v3. Separately, mount has .hostNodes() if needed.

I'm going to close this, but can certainly reopen it if there's value and it can be rebased.

Brandon Dail added 3 commits May 10, 2016 21:06

Added failing test case

da4ae2e

Use inst when isDOMComponent(inst) is false

a35a501

Use inst in instHasProperty as well

14b92f5

aweary changed the title ~~Parse props in instMatchesObjectProps for all instances~~ Parse props in instMatchesObjectProps/instHasProperty for all instances May 11, 2016

Brandon Dail added 2 commits May 10, 2016 21:31

Uncomment expect call in test

dad11fd

Use ternary operator in instHasProperty as well

0c24788

blainekasten added API: mount semver: patch labels May 11, 2016

aweary mentioned this pull request May 11, 2016

Expand test coverage for composite components #380

Open

Merge branch 'master' into mounted-find-props

a8ca106

aweary mentioned this pull request May 27, 2016

tests for shallow#props() and prop() #420

Merged

aweary mentioned this pull request Jun 8, 2016

Clarify shallow rendering behavior with props #445

Open

Brandon Dail added 2 commits June 9, 2016 08:25

Merge branch 'master' into mounted-find-props

0e1b039

Only query composites when using object syntax

b38e3b6

ljharb assigned lelandrichardson Jul 5, 2016

aweary mentioned this pull request Jul 5, 2016

Remove arbitrary props from DOM elements #487

Merged

travisperson mentioned this pull request Aug 12, 2016

Fix Mount CSS prop selector for parity with Shallow #538

Closed

aweary mentioned this pull request Sep 6, 2016

Object Property Selector for Full DOM Rendering doesn't find non-DOM components #582

Closed

ljharb closed this Aug 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse props in instMatchesObjectProps/instHasProperty for all instances #377

Parse props in instMatchesObjectProps/instHasProperty for all instances #377

aweary commented May 11, 2016

lelandrichardson commented May 11, 2016

lelandrichardson commented May 11, 2016 •

edited by aweary

Loading

blainekasten commented May 11, 2016

nfcampos commented May 11, 2016

aweary commented May 11, 2016

nfcampos commented May 11, 2016

lelandrichardson commented May 11, 2016

aweary commented May 11, 2016 •

edited

Loading

nfcampos commented May 11, 2016

lelandrichardson commented May 11, 2016

nfcampos commented May 11, 2016

aweary commented May 11, 2016

lelandrichardson commented May 11, 2016

aweary commented May 11, 2016 •

edited

Loading

lelandrichardson commented May 11, 2016

aweary commented May 11, 2016

nfcampos commented May 11, 2016

lelandrichardson commented May 11, 2016

nfcampos commented May 11, 2016

aweary commented May 11, 2016 •

edited

Loading

nfcampos commented May 11, 2016

aweary commented May 11, 2016 •

edited

Loading

nfcampos commented May 11, 2016

aweary commented Jun 9, 2016

aweary commented Jul 5, 2016

lelandrichardson commented Jul 8, 2016

MrLeebo commented Jan 3, 2017 •

edited

Loading

ljharb commented Aug 7, 2018

Parse props in instMatchesObjectProps/instHasProperty for all instances #377

Parse props in instMatchesObjectProps/instHasProperty for all instances #377

Conversation

aweary commented May 11, 2016

lelandrichardson commented May 11, 2016

lelandrichardson commented May 11, 2016 • edited by aweary Loading

blainekasten commented May 11, 2016

nfcampos commented May 11, 2016

aweary commented May 11, 2016

nfcampos commented May 11, 2016

lelandrichardson commented May 11, 2016

aweary commented May 11, 2016 • edited Loading

nfcampos commented May 11, 2016

lelandrichardson commented May 11, 2016

nfcampos commented May 11, 2016

aweary commented May 11, 2016

lelandrichardson commented May 11, 2016

aweary commented May 11, 2016 • edited Loading

lelandrichardson commented May 11, 2016

aweary commented May 11, 2016

nfcampos commented May 11, 2016

lelandrichardson commented May 11, 2016

nfcampos commented May 11, 2016

aweary commented May 11, 2016 • edited Loading

nfcampos commented May 11, 2016

aweary commented May 11, 2016 • edited Loading

nfcampos commented May 11, 2016

aweary commented Jun 9, 2016

aweary commented Jul 5, 2016

lelandrichardson commented Jul 8, 2016

MrLeebo commented Jan 3, 2017 • edited Loading

TLDR: How about wrapper.find(..., {includeNonDom: true}) and deprecate selectors targeting non-DOM components when mounted?

ljharb commented Aug 7, 2018

lelandrichardson commented May 11, 2016 •

edited by aweary

Loading

aweary commented May 11, 2016 •

edited

Loading

aweary commented May 11, 2016 •

edited

Loading

aweary commented May 11, 2016 •

edited

Loading

aweary commented May 11, 2016 •

edited

Loading

MrLeebo commented Jan 3, 2017 •

edited

Loading

TLDR: How about `wrapper.find(..., {includeNonDom: true})` and deprecate selectors targeting non-DOM components when mounted?