[Fix] Convert nodes to RST nodes before comparing #1423
Conversation
packages/enzyme/src/ReactWrapper.js
Outdated
| */ | ||
| containsAnyMatchingElements(nodes) { | ||
| return Array.isArray(nodes) && nodes.some(node => this.containsMatchingElement(node)); | ||
| const rstNodes = nodes.map(getAdapter().elementToNode); |
There was a problem hiding this comment.
this change makes .map be called before Array.isArray is called to ensure it's an array.
Let's change this to if (!Array.isArray(nodes)) { return false; } at the top of the function, and then we can do the entire check in a single some
| const rstNodeOrNodes = isArray | ||
| ? nodeOrNodes.map(getAdapter().elementToNode) | ||
| : getAdapter().elementToNode(nodeOrNodes); | ||
| const predicate = Array.isArray(nodeOrNodes) |
There was a problem hiding this comment.
this line can use isArray now, I think?
packages/enzyme/src/ReactWrapper.js
Outdated
| const predicate = Array.isArray(nodeOrNodes) | ||
| ? other => containsChildrenSubArray(nodeEqual, other, nodeOrNodes) | ||
| : other => nodeEqual(nodeOrNodes, other); | ||
| ? other => containsChildrenSubArray(nodeEqual, other, rstNodeOrNodes) |
There was a problem hiding this comment.
probably at this point, rather than two paired multiline ternaries, this would be better as an if/else branch.
packages/enzyme/src/ReactWrapper.js
Outdated
| matchesElement(node) { | ||
| return this.single('matchesElement', () => nodeMatches(node, this.getNodeInternal(), (a, b) => a <= b)); | ||
| const rstNode = getAdapter().elementToNode(node); | ||
| return this.single('matchesElement', () => nodeMatches(rstNode, this.getNodeInternal(), (a, b) => a <= b)); |
There was a problem hiding this comment.
let's do the elementToNode conversion here inside the arrow function, so that it happens after this.single succeeds.
ljharb
requested review from
a team,
aweary,
blainekasten,
lelandrichardson and
nfcampos
|
@eddyerburgh is this change not needed in |
|
@ljharb Yes, you're right. I've only been implementing mount so haven't looked into shallow. I'll add the changes now |
|
It was already implemented for contains in ShallowWrapper, so I refactored I also realized Finally, while I'm here, in |
packages/enzyme/src/ReactWrapper.js
Outdated
| ? other => containsChildrenSubArray( | ||
| nodeEqual, | ||
| other, | ||
| nodeOrNodes.map(adapter.elementToNode), |
There was a problem hiding this comment.
Let’s make this use an arrow function, so adapters only get the single argument
There was a problem hiding this comment.
I also see this is how it already is in https://github.com/airbnb/enzyme/pull/1423/files#diff-fa27c82ee4fde075bae6173848e17c82R448; let’s change both
There was a problem hiding this comment.
Let’s make this use an arrow function, so adapters only get the single argument
Do you mean the nodeOrNodes map callback should be an arrow function?
There was a problem hiding this comment.
yep! like .map(x => adapter.elementToNode(x))
This was done intentionally, so there couldn’t be any drift between how these three methods worked - please keep them linked. |
|
In |
Sorry, I meant that I removed the unnecessary calls to |
|
gotcha, sounds good then :-) |
|
Thanks @ljharb . Is there anything else you need me to do to get this merged? Also, do you know when this will be released? |
|
@eddyerburgh at the moment, you're good; i'm waiting for other collaborators to review it first. After it's merged, it will be released whenever we're ready to publish the next one :-) hopefully soon. |
|
Could we get this reviewed and merged? |
|
The following packages are now released: |
3 participants
Convert nodes in
containsfunctions to RST before comparingFixes Add RST mapping to contains #1422