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.

Sign up for GitHub

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

Jump to bottom

[Fix] Convert nodes to RST nodes before comparing #1423

Merged
merged 5 commits into from
Jan 19, 2018
Merged

[Fix] Convert nodes to RST nodes before comparing #1423

merged 5 commits into from
Jan 19, 2018

Conversation

eddyerburgh
Copy link
Contributor

@eddyerburgh eddyerburgh commented Dec 8, 2017 •
edited

@eddyerburgh eddyerburgh mentioned this pull request Dec 8, 2017
Closed
@eddyerburgh eddyerburgh mentioned this pull request Dec 8, 2017
Closed
ljharb
ljharb requested changes Dec 9, 2017
View reviewed changes
packages/enzyme/src/ReactWrapper.js Outdated
@@ -446,7 +454,10 @@ class ReactWrapper {
* @returns {Boolean}
*/
containsAnyMatchingElements(nodes) {
return Array.isArray(nodes) && nodes.some(node => this.containsMatchingElement(node));
const rstNodes = nodes.map(getAdapter().elementToNode);
Copy link
Member

Choose a reason for hiding this comment

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

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

packages/enzyme/src/ReactWrapper.js Outdated
const isArray = Array.isArray(nodeOrNodes);
const rstNodeOrNodes = isArray
? nodeOrNodes.map(getAdapter().elementToNode)
: getAdapter().elementToNode(nodeOrNodes);
const predicate = Array.isArray(nodeOrNodes)
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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
@@ -358,7 +358,8 @@ class ReactWrapper {
* @returns {Boolean}
*/
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));
Copy link
Member

Choose a reason for hiding this comment

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

let's do the elementToNode conversion here inside the arrow function, so that it happens after this.single succeeds.

ljharb
ljharb approved these changes Dec 9, 2017
View reviewed changes
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.

Seems reasonable.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2017

@eddyerburgh is this change not needed in shallow as well?

@eddyerburgh
Copy link
Contributor Author

@ljharb Yes, you're right. I've only been implementing mount so haven't looked into shallow. I'll add the changes now

@eddyerburgh
Copy link
Contributor Author

It was already implemented for contains in ShallowWrapper, so I refactored contains in ReactWrapper.

I also realized containsAllMatchingElements and containsAnyMatchingElements call containsMatchingElement, so I removed the unncessary calls to containsMatchingElement.

Finally, while I'm here, in ReactWrapper.contains there's no check that the node/nodes is a React element. Should I add a test and write a check, like there is in ShallowWrapper.contains?

ljharb
ljharb reviewed Dec 9, 2017
View reviewed changes
packages/enzyme/src/ReactWrapper.js Outdated
? other => containsChildrenSubArray(
nodeEqual,
other,
nodeOrNodes.map(adapter.elementToNode),
Copy link
Member

Choose a reason for hiding this comment

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

Let’s make this use an arrow function, so adapters only get the single argument

Copy link
Member

Choose a reason for hiding this comment

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

I also see this is how it already is in https://github.com/airbnb/enzyme/pull/1423/files#diff-fa27c82ee4fde075bae6173848e17c82R448; let’s change both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

yep! like .map(x => adapter.elementToNode(x))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🙂

@ljharb
Copy link
Member

ljharb commented Dec 9, 2017

I also realized containsAllMatchingElements and containsAnyMatchingElements call containsMatchingElement, so I removed the unncessary calls to containsMatchingElement.

This was done intentionally, so there couldn’t be any drift between how these three methods worked - please keep them linked.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2017

In mount, someone might pass a DOM node; I’m not sure what the best solution is there.

@eddyerburgh
Copy link
Contributor Author

so I removed the unncessary calls to containsMatchingElement

Sorry, I meant that I removed the unnecessary calls to elementToNode. They are all still linked.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2017

gotcha, sounds good then :-)

ljharb
ljharb approved these changes Dec 9, 2017
View reviewed changes
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!

@eddyerburgh
Copy link
Contributor Author

Thanks @ljharb . Is there anything else you need me to do to get this merged?

Also, do you know when this will be released?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2017

@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.

@eddyerburgh
Copy link
Contributor Author

eddyerburgh commented Jan 18, 2018 •
edited

Could we get this reviewed and merged?

@ljharb ljharb merged commit 73334a7 into enzymejs:master Jan 19, 2018
@ljharb
Copy link
Member

ljharb commented Aug 8, 2018

The following packages are now released:

This was referenced Aug 8, 2018
Open
Merged
Closed
Closed
This was referenced Oct 1, 2018
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@brucewpaul brucewpaul brucewpaul approved these changes

@nfcampos nfcampos Awaiting requested review from nfcampos

@lelandrichardson lelandrichardson Awaiting requested review from lelandrichardson

@blainekasten blainekasten Awaiting requested review from blainekasten

@aweary aweary Awaiting requested review from aweary

Assignees
No one assigned
Labels
API: mount API: shallow semver: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eddyerburgh @ljharb @brucewpaul