Do not return false positive when passing function (and other non-Rea… #259
Conversation
| </div> | ||
| ); | ||
|
|
||
| expect(wrapper.contains(functionalComponent)).to.equal(false); |
There was a problem hiding this comment.
I'm confused - in this case, the wrapper does contain functionalComponent - shouldn't this return true?
There was a problem hiding this comment.
@ljharb part of the JSX spec is that lowercase component names (ie, <functionalComponent> should be interpreted as string "DOM" components).
There was a problem hiding this comment.
@ljharb That was what I thought at first, but in this case functionalComponent is not a ReactElement (which would be a valid argument for contains) but a render function.
Unfortunately it is not possible for now to use selector as contains argument so this test could pass (I dont have time now to confirm that, but AFAIR it should) if we make such assertion - expect(wrapper.contains(<functionalComponent />)).to.equal(false) .
But due to presence of that text node the function would return (before my PR) true, although wouldn't return true for just shallow(<div><functionalComponent></div>). Therefore it's outcome was kinda unpredictable and cost me some time as I didn't read the docs carefully and thought that using selector here is OK (still - I would love to be able to use selectors here too ;) )
@lelandrichardson
True that, gonna amend this commit in a minute
There was a problem hiding this comment.
I'm not sure how that changes this shallow rendering case tho - functionalComponent here is an SFC. Whether it's upper or lower case, why would anyone expect the wrapper not to contain something they just rendered in it?
There was a problem hiding this comment.
aha - you're saying .contains(<functionalComponent />) would be true, but .contains(functionalComponent) should be false, because it's not an instance-backed React component?
There was a problem hiding this comment.
Yes, that's what I'm talking about 👍
I would like to fix it, but:
- I don't know why it was done like that - I think somebody had some logic in mind with that, so first we need to establish that - why only ReactElements or an array of them is allowed as argument and it differs in logic so much from
find? - I might be a little too greenhorn in enzyme yet to make such a big change in a codebase and its logic
|
I misunderstood this PR at first, but I agree this is a good thing. The problem is some people might confuse what kind of an argument We should actually probably just throw when someone passes something into |
|
Well, I might do that as well - it would be for sure more verbose and would save some time. Should I just change this PR to throw on any argument passed in except of:
Somehow just tell me it will still be possible to break it in some other case with SFC. Gonna maybe try to break it later |
|
@Andarist actually, |
|
Ye, u r right ofc, I've just forgot to mention it :) Anyway - is it suitable solution for enzyme to throw on invalid arguments in this case? Should I proceed with implementing it? |
|
Yeah I think it's a good idea - it surfaces the error quickly to the user, and decreases the chance of people writing tests that pass without actually testing what they mean to test. |
|
Ok, I've pushed amended version, please take a look at it ;) |
| } | ||
|
|
||
| export function isReactElementAlike(arg) { | ||
| return React.isValidElement(arg) || isTextualNode(arg) || Array.isArray(arg); |
There was a problem hiding this comment.
👍 one thing that's still not great... a very common mistake will be passing in a string selector: ie .contains('.button-class') in such a way that this won't throw, since text is a valid node.
Not sure what we can do about that at this point though...
There was a problem hiding this comment.
Well, it is for sure a common issue for people that they assume selectors are allowed here.
Maybe we should allow them and try to merge those 2 different logics of selectors and nodeEquals into one? Although I ain't sure if that's even possible elegantly.
|
@Andarist do you mind rebasing this? Looks like it has some conflicts but I think this should go in. Thanks. |
|
@lelandrichardson sure thing, will do 2day |
… return false positive when passing function (and other non-React elements types) into contains method
|
done, feel free to merge :) |
…ct elements types) into contains method