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

Skip undefined props when comparing nodes #662

Merged
merged 2 commits into from
Sep 26, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Breaking] node comparisons should ignore undefined props
 - `.find` with an object with `undefined` values should throw

Fixes #656.
  • Loading branch information
vdemedes authored and ljharb committed Nov 8, 2016
commit b139bcbe7b2ae2ae1a68e40822041b136abdd506
2 changes: 1 addition & 1 deletion docs/api/ShallowWrapper/equals.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ expect(wrapper.equals(<div className="foo bar" />)).to.equal(true);
when you are calling it you are calling it with a ReactElement or a JSX expression.
- Keep in mind that this method determines equality based on the equality of the node's children as
well.

- Following React's behavior, `.equals()` ignores properties whose values are `undefined`.
15 changes: 13 additions & 2 deletions docs/api/selector.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@ a string can be used to find it:
function MyComponent() {
return <div />;
}
MyComponent.displayName = 'MyComponent!';
MyComponent.displayName = 'My Component';

// find instances of MyComponent
const myComponents = wrapper.find('MyComponent!');
const myComponents = wrapper.find('My Component');
```

NOTE: This will *only* work if the selector (and thus the component's `displayName`) is a string
Expand All @@ -113,3 +113,14 @@ wrapper.find({ foo: 3 });
wrapper.find({ bar: false });
wrapper.find({ title: 'baz' });
```

**Note - undefined properties**
are not allowed in the object property selector and will cause an error:


```jsx
wrapper.find({ foo: 3, bar: undefined });
// => TypeError: Enzyme::Props can't have 'undefined' values. Try using 'findWhere()' instead.
```

If you have to search by `undefined` property value, use [.findWhere()](ShallowWrapper/findWhere.md).
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,5 +104,6 @@
"sinon": "^2.4.1",
"webpack": "^1.13.3"
},
"dependencies": {}
"dependencies": {
}
}
14 changes: 13 additions & 1 deletion packages/enzyme-test-suite/test/ReactWrapper-spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,19 @@ describeWithDOM('mount', () => {
expect(wrapper.find('[htmlFor]')).to.have.length(2);
});

it('should error sensibly if any of the search props are undefined', () => {
const wrapper = mount((
<div>
<input type={undefined} />
</div>
));

expect(() => wrapper.find({ type: undefined })).to.throw(
TypeError,
'Enzyme::Props can’t have `undefined` values. Try using ‘findWhere()’ instead.',
);
});

it('should compound tag and prop selector', () => {
const wrapper = mount(
<div>
Expand Down Expand Up @@ -655,7 +668,6 @@ describeWithDOM('mount', () => {
expect(wrapper.find({ a: 1 })).to.have.length(0);
expect(wrapper.find({ 'data-test': 'ref' })).to.have.length(7);
expect(wrapper.find({ className: 'foo' })).to.have.length(1);
expect(wrapper.find({ 'data-prop': undefined })).to.have.length(1);
expect(wrapper.find({ 'data-prop': null })).to.have.length(1);
expect(wrapper.find({ 'data-prop': 123 })).to.have.length(1);
expect(wrapper.find({ 'data-prop': false })).to.have.length(1);
Expand Down
14 changes: 13 additions & 1 deletion packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,19 @@ describe('shallow', () => {
);
});

it('should error sensibly if any of the search props are undefined', () => {
const wrapper = shallow(
<div>
<input type={undefined} />
</div>,
);

expect(() => wrapper.find({ type: undefined })).to.throw(
TypeError,
'Enzyme::Props can’t have `undefined` values. Try using ‘findWhere()’ instead.',
);
});

it('should compound tag and prop selector', () => {
const wrapper = shallow(
<div>
Expand Down Expand Up @@ -695,7 +708,6 @@ describe('shallow', () => {
expect(wrapper.find({ a: 1 })).to.have.length(0);
expect(wrapper.find({ 'data-test': 'ref' })).to.have.length(7);
expect(wrapper.find({ className: 'foo' })).to.have.length(1);
expect(wrapper.find({ prop: undefined })).to.have.length(1);
expect(wrapper.find({ prop: null })).to.have.length(1);
expect(wrapper.find({ prop: 123 })).to.have.length(1);
expect(wrapper.find({ prop: false })).to.have.length(1);
Expand Down
7 changes: 7 additions & 0 deletions packages/enzyme-test-suite/test/Utils-spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ describe('Utils', () => {
)).to.equal(false);
});

it('should skip undefined props', () => {
expect(nodeEqual(
<div id="foo" className={undefined} />,
<div id="foo" />,
)).to.equal(true);
});

it('should check children as well', () => {
expect(nodeEqual(
<div>
Expand Down
1 change: 1 addition & 0 deletions packages/enzyme/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"object-is": "^1.0.1",
"object.assign": "^4.0.4",
"object.entries": "^1.0.4",
"object.values": "^1.0.4",
"raf": "^3.3.2",
"rst-selector-parser": "^2.2.1"
},
Expand Down
12 changes: 11 additions & 1 deletion packages/enzyme/src/RSTTraversal.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import flatten from 'lodash/flatten';
import entries from 'object.entries';
import isSubset from 'is-subset';
import functionName from 'function.prototype.name';
import { nodeHasProperty } from './Utils';
Expand Down Expand Up @@ -98,8 +99,17 @@ export function nodeHasId(node, id) {

export { nodeHasProperty };

const CAN_NEVER_MATCH = {};
function replaceUndefined(v) {
return typeof v !== 'undefined' ? v : CAN_NEVER_MATCH;
}
function replaceUndefinedValues(obj) {
return entries(obj)
.reduce((acc, [k, v]) => ({ ...acc, [k]: replaceUndefined(v) }), {});
}

export function nodeMatchesObjectProps(node, props) {
return isSubset(propsOfNode(node), props);
return isSubset(propsOfNode(node), replaceUndefinedValues(props));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t understand why we need this change. Don’t we throw if the received props contain undefined values?

Copy link
Member

Choose a reason for hiding this comment

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

In find, yes, but I don't think that's the only place this is called.

}

export function getTextFromNode(node) {
Expand Down
4 changes: 3 additions & 1 deletion packages/enzyme/src/Utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ export function isCustomComponentElement(inst, adapter) {
}

function propsOfNode(node) {
return (node && node.props) || {};
return entries((node && node.props) || {})
.filter(([, value]) => typeof value !== 'undefined')
.reduce((acc, [key, value]) => ({ ...acc, [key]: value }), {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rather ineffecient, creating as many objects as you have keys, I know this is a testing library, but if you have a lot of tests it starts to add up

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I could mutate the accumulator since the overall reduce is pure.

Copy link
Member

Choose a reason for hiding this comment

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

Done.

}

export function typeOfNode(node) {
Expand Down
5 changes: 5 additions & 0 deletions packages/enzyme/src/selectors.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createParser } from 'rst-selector-parser';
import values from 'object.values';
import isEmpty from 'lodash/isEmpty';
import flatten from 'lodash/flatten';
import unique from 'lodash/uniq';
Expand Down Expand Up @@ -141,6 +142,10 @@ export function buildPredicate(selector) {
// If the selector is an non-empty object, treat the keys/values as props
if (typeof selector === 'object') {
if (!Array.isArray(selector) && selector !== null && !isEmpty(selector)) {
const hasUndefinedValues = values(selector).some(value => value === undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this one a test of the value itself but the others are testing typeof?

Copy link
Member

Choose a reason for hiding this comment

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

My habit is to always use typeof for safety; babel makes this unnecessary. I'll make them all be the same.

Copy link
Member

Choose a reason for hiding this comment

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

It's now consistent in the project.

if (hasUndefinedValues) {
throw new TypeError('Enzyme::Props can’t have `undefined` values. Try using ‘findWhere()’ instead.');
}
return node => nodeMatchesObjectProps(node, selector);
}
throw new TypeError(
Expand Down