Skip undefined props when comparing nodes #662

vadimdemedes · 2016-11-06T10:35:25Z

Fixes #656.

This PR ensures null and undefined props are skipped when comparing nodes:

expect(nodeEqual(
   <div className={null} id={undefined} />,
   <div />
)).to.equal(true);

A helper function cleanProps removes all null and undefined props. I wanted to do that in propsOfNode(), but there are multiple return statements, which would result in a lot of repetition.

Let me know if this PR needs modification or anything else ;)

ljharb

I think doing this in propsOfNode is the proper way, since a lot of places call that - repetition inside an abstraction is OK.

Tests LGTM tho!

ljharb · 2016-11-06T17:23:06Z

src/Utils.js

  for (let i = 0; i < leftKeys.length; i += 1) {
    const prop = leftKeys[i];
    // we will check children later
-    if (prop === 'children') {
+    if (prop === 'children' || !left[prop]) {


shouldn't this be left[prop] == null, since 0 is falsy too?

Ahh, leftover from debugging.

ljharb · 2016-11-06T17:23:20Z

src/Utils.js

@@ -1,5 +1,6 @@
 /* eslint no-use-before-define:0 */
 import isEqual from 'lodash/isEqual';
+import isNil from 'lodash/isNil';


i'm not sure why we need a dependency for == null?

I assumed the linter won't like non-strict equality check, I should've checked, my bad!

a non-strict check against null is the sole exception :-)

ljharb · 2016-11-06T17:23:38Z

src/Utils.js

@@ -114,7 +115,7 @@ export function nodeEqual(a, b, lenComp = is) {
  }

  if (!isTextualNode(a)) {
-    const rightKeys = Object.keys(right);
+    const rightKeys = Object.keys(right).filter(key => !isNil(right[key]));


let's just do key => right[key] != null - it's clearer and built-in.

Sure, agree.

vadimdemedes · 2016-11-06T18:47:43Z

Apparently, moving cleanProps() to propsOfNode() breaks these tests:

I guess cleanProps() should stay inside nodeEqual(). @ljharb What do you think?

ljharb · 2016-11-06T18:50:18Z

If we're going to ignore null, we need to ignore it consistently. In other words, searching for a value of null should be the same as a value of undefined, or as "absence".

In other words, if we're asking if a node has property "foo" equal to null, and it doesn't have the property "foo", then I'd expect it to pass.

vadimdemedes · 2016-11-06T18:58:23Z

Ok, I see your point!

Regarding this test: https://github.com/airbnb/enzyme/blob/master/test/ShallowWrapper-spec.jsx#L577, let me clarify one last thing. If there are nodes with props equal to null or undefined and we're looking for them (like in the test), these nodes shouldn't be returned, because we treat those props as missing.

expect(wrapper.find({ prop: undefined })).to.have.length(0);
expect(wrapper.find({ prop: null })).to.have.length(0);

ljharb · 2016-11-06T19:00:35Z

yeah, that's a good point/wrinkle. It seems like the options are, when finding { foo: null, bar: 3 }, to find anything that omits foo and has bar equal to 3 - or, to simply find anything that has bar equal to 3.

If we choose the former path, .find({ prop: null }) will find everything. If we choose the latter, .find({ prop: null }) will find nothing. I'm not sure if either makes sense.

vadimdemedes · 2016-11-06T19:05:12Z

I think there's a 3rd option. Check props in find() and throw if there are null or undefined values.

find(props) {
  const hasNilProps = Object.keys(props).some(key => props[key] == null);

  if (hasNilProps) {
    throw new TypeError('Props can\'t have `null` or `undefined` values.');
  }

  // ...
}

What do you think?

ljharb · 2016-11-06T19:06:43Z

I think that's probably best, but then that means that it's a breaking change.

@lelandrichardson @aweary @blainekasten @nfcampos what do you think?

nfcampos · 2016-11-06T19:18:42Z

this 3rd option is definitely the best of the three. this change should be accompanied with a note on the docs for find() saying that if you want to find nodes whose props are null you can use findWhere. maybe even the error message should mention this. what do you think?

vadimdemedes · 2016-11-07T07:31:17Z

I think @nfcampos's proposal is reasonable and makes this change clear to users.

aweary · 2016-11-07T15:18:33Z

I think @nfcampos's proposal is reasonable and makes this change clear to users.

Agreed 👍

vadimdemedes · 2016-11-07T15:22:41Z

Glad we agree, I'll update the PR!

ljharb · 2016-11-08T07:51:02Z

ok, so it looks like React ignores undefined but not null - http://stackoverflow.com/questions/34809178/react-js-default-prop-is-not-used-with-null-is-passed - so we should change this PR to continue to treat null as a valid, existing value.

vadimdemedes · 2016-11-08T10:52:03Z

@ljharb Good catch, will do!

- `.find` with an object with `undefined` values should throw Fixes enzymejs#656.

vadimdemedes · 2016-11-08T13:55:38Z

PR updated, addressing all of the feedback above. Thanks to everyone!

One question: I put the warning about an error when using .find() and having some undefined props in the docs/selector.md. Should I put it in API docs for .find() or leave it there?

nfcampos

This looks great @vdemedes just please add the missing trailing commas to appease the linter and make travis green :)

nfcampos · 2016-11-12T11:38:29Z

docs/api/ShallowWrapper/equals.md

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


please remove the comma after 'properties'

vadimdemedes · 2016-11-12T12:47:08Z

All good now, thanks guys for the feedback ;)

ljharb · 2016-11-12T16:42:31Z

src/ComplexSelector.js

@@ -78,6 +78,13 @@ export default class ComplexSelector {
      return this.handleSelectors(selectors, wrapper);
    }

+    if (typeof selector === 'object' && selector !== null) {
+      const hasUndefinedValues = Object.keys(selector).some(key => selector[key] === undefined);


import values from 'object.values' and you can do values(selector).some(value => value === undefined)

Good suggestion, updated!

ljharb · 2016-11-12T18:26:38Z

src/Utils.js

+      newProps[key] = props[key];
+    });
+
+  return newProps;


alternatively:

return entries(props) .filter(([, value]) => value !== undefined) .reduce((acc, [key, value]) => assign({}, acc, { [key]: value }), {});

nfcampos · 2017-08-31T18:22:17Z

@ljharb should we get this in for v3?

ljharb · 2017-09-23T15:37:38Z

I've rebased this on top of latest master; @aweary @nfcampos would you mind rereviewing?

needs a rereview

nfcampos

Some comments

nfcampos · 2017-09-24T15:32:07Z

packages/enzyme/src/selectors.js

@@ -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);


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

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

It's now consistent in the project.

nfcampos · 2017-09-24T15:33:20Z

packages/enzyme/src/Utils.js

-  return (node && node.props) || {};
+  return entries((node && node.props) || {})
+    .filter(([, value]) => typeof value !== 'undefined')
+    .reduce((acc, [key, value]) => ({ ...acc, [key]: value }), {});


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

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

nfcampos · 2017-09-24T15:34:54Z

packages/enzyme/src/RSTTraversal.js

 export function nodeMatchesObjectProps(node, props) {
-  return isSubset(propsOfNode(node), props);
+  return isSubset(propsOfNode(node), replaceUndefinedValues(props));


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

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

ljharb requested changes Nov 6, 2016

View reviewed changes

ljharb added API: shallow API: mount semver: patch labels Nov 6, 2016

vadimdemedes changed the title ~~Skip null or undefined props when comparing nodes~~ Skip undefined props when comparing nodes Nov 8, 2016

vadimdemedes force-pushed the skip-null-or-undefined-props branch from 379ea7c to 034cc0c Compare November 8, 2016 13:33

[Breaking] node comparisons should ignore undefined props

b139bcb

- `.find` with an object with `undefined` values should throw Fixes enzymejs#656.

vadimdemedes force-pushed the skip-null-or-undefined-props branch from 34a6d8c to 886e90d Compare November 10, 2016 14:29

nfcampos previously approved these changes Nov 12, 2016

View reviewed changes

vadimdemedes force-pushed the skip-null-or-undefined-props branch from 886e90d to 4827455 Compare November 12, 2016 12:19

ljharb approved these changes Nov 12, 2016

View reviewed changes

ljharb added semver: major Breaking changes and removed semver: patch labels Nov 12, 2016

vadimdemedes force-pushed the skip-null-or-undefined-props branch from 4827455 to 41b0c8a Compare November 12, 2016 16:47

ljharb reviewed Nov 12, 2016

View reviewed changes

vadimdemedes force-pushed the skip-null-or-undefined-props branch from 41b0c8a to ae25d12 Compare November 13, 2016 13:29

vadimdemedes force-pushed the skip-null-or-undefined-props branch from ae25d12 to e0c3c45 Compare November 13, 2016 13:30

ljharb mentioned this pull request Nov 17, 2016

Fix falsy attributes being ignored #459

Closed

ljharb force-pushed the skip-null-or-undefined-props branch from e0c3c45 to eca06ae Compare September 23, 2017 15:37

ljharb requested review from aweary and a team September 23, 2017 15:38

nfcampos reviewed Sep 24, 2017

View reviewed changes

Review feedback

9bb8a91

lelandrichardson approved these changes Sep 26, 2017

View reviewed changes

ljharb force-pushed the skip-null-or-undefined-props branch from eca06ae to 9bb8a91 Compare September 26, 2017 03:31

ljharb merged commit efb3913 into enzymejs:master Sep 26, 2017

Skip undefined props when comparing nodes #662

Skip undefined props when comparing nodes #662

Conversation

vadimdemedes commented Nov 6, 2016

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Nov 6, 2016 • edited Loading

ljharb commented Nov 6, 2016

vadimdemedes commented Nov 6, 2016

ljharb commented Nov 6, 2016

vadimdemedes commented Nov 6, 2016

ljharb commented Nov 6, 2016

nfcampos commented Nov 6, 2016

vadimdemedes commented Nov 7, 2016

aweary commented Nov 7, 2016

vadimdemedes commented Nov 7, 2016

ljharb commented Nov 8, 2016

vadimdemedes commented Nov 8, 2016

vadimdemedes commented Nov 8, 2016

nfcampos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Nov 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfcampos commented Aug 31, 2017

ljharb commented Sep 23, 2017

nfcampos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Nov 6, 2016 •

edited

Loading