add support for object property selector #110

hartzis · 2016-01-11T03:02:55Z

@blainekasten, thanks for quoting me in #105, gave me the drive to attempt to add the support. Let me know what you guys think.

I also just happened to notice while I tried to follow code patterns adding functionality that there seems to be some opportunity to reuse some code between MountedTraversal and ShallowTraversal. There was definitely some copy & paste I did :)

blainekasten · 2016-01-11T04:48:27Z

src/ShallowTraversal.js

@@ -95,6 +96,10 @@ export function nodeHasType(node, type) {
  return node.type.name === type || node.type.displayName === type;
 }

+export function nodeHasProps(node, props) {


This function name clashes quite a lot with the nodeHasProperty function. I think it should probably be more explicit so it's known that it's meant for the object selector.

Something like nodeMatchesObjectProps ?

I like that name a lot more!

hartzis · 2016-01-12T00:55:22Z

Updated with suggested edits.

blainekasten · 2016-01-12T17:08:12Z

src/MountedTraversal.js

+export function instMatchesObjectProps(inst, props) {
+  if (!isDOMComponent(inst)) return false;
+  const node = getNode(inst);
+  return !isEmpty(props) && isMatch(propsOfNode(node), props);


the !isEmpty check seems redudant. On L318, you are ensuring that props can't be null.

So it seems like the isEmpty call is just a perf concern to make sure users aren't doing find({}), but that should be the minority case.

So you are actually just adding complexity for the typical case of find({foo: bar}).

Ultimately, I think the isEmpty call is superflous and negatively affecting performance in a very small way.

I don't see a line 318 in this diff.

what does isMatch(propsOfNode(node), {}) do?

L218. My bad.

isMatch compares keys and values of two objects to be equal

right, so, if propsOfNode(node) are empty, and props are empty, then as-is, it returns false - with your change, it returns true.

Which is desired?

That is true. But how often is somebody going to look for something with an empty object? That just seems like a design against the wrong use-case. And I guess maybe somebody does want to search for nodes that don't have any props? which find({}) could support? feels awkward though. I guess I could see either way here.

That seems reasonable (to throw an error). @lelandrichardson, thoughts?

if they are actually passing in an empty object, then matching all nodes actually seems like the reasonable response to me?

I agree this doesn't seem like a normal use case, although I can imagine some code that builds up some object dynamically, and in some cases might build up to nothing...

I don't feel strongly about this, but I don't see the harm in having {} match all nodes. It seems like the expected behavior, despite being a "corner-case" type input?

I agree this doesn't seem like a normal use case, although I can imagine some code that builds up some object dynamically, and in some cases might build up to nothing...

Exactly. But I would say that's a bug in their code. If they are dynamically building an object, and it gets to an empty state. An error should be thrown, so they can make sure they are testing what they want.

I don't feel strongly about this, but I don't see the harm in having {} match all nodes. It seems like the expected behavior, despite being a "corner-case" type input?

Would you also want to see find('[]') to match every node? Because that is the same API effectively.

Comparing it to the '[]' api, I would think it should only find components with props that match the specified key/values.

It seems its 3-against-1, so I'm happy to go with the approach of throwing an error for this case.

I'll update this after we discuss the best solution to the nested objects below.

lelandrichardson · 2016-01-15T00:22:03Z

This looks great. Quick question, though:

isMatch makes a strict equality check on the props of the object passed in. This means that the following will not work:

const wrapper = shallow(<div style={{ height: 20 }} />);
expect(wrapper.is({ style: { height: 20 } })).to.equal(true);

Do we potentially want to implement our own matcher? We may even want to have special flattening for the style prop.

hartzis · 2016-01-15T01:16:12Z

Yeah style poses an interesting problem. Also just general nested objects { prop: { id: 1 } }.

isMatch was used because underscore is already included in the library.

is-subset is used by skin-deep to compare prop objects. What about using is-subset?

ljharb · 2016-01-15T04:18:01Z

I also made is-equal which works a bit more reliably than assert.deepEqual and is used by expect. It doesn't work for subsets though.

hartzis · 2016-01-17T15:48:24Z

@ljharb Just took a look at is-equal, very nice library, but i think you're right, since it doesn't work for subsets I decided to give is-subset a whirl.

Let me know how you guys feel about this possible solution.

hartzis · 2016-01-17T15:49:34Z

src/__tests__/ReactWrapper-spec.js

+      expect(wrapper.find({ more: { a: 1 } })).to.have.length(0);
+      expect(wrapper.find({ more: [{ id: 1 }] })).to.have.length(2);
+      expect(wrapper.find({ more: { item: { id: 1 } } })).to.have.length(1);
+      expect(wrapper.find({ style: { height: 20 } })).to.have.length(1);


@lelandrichardson This works now with is-subset, what do you think?

lelandrichardson · 2016-01-17T18:22:12Z

I was trying to think about what the most sensible API for this is... I think there are 2 questions:

Since we are effectively doing "subset" matching behavior one level down for props, should we continue to match things as subsets at the sub-prop level?
Since style has special built-in behavior, do we want to handle it differently by pre-flattening the styles array for matching?

Question 2 is sort of a special case of Question 1... it just goes a bit further. At the moment I feel like the answer to 2 is "yes", and if 2 is "yes" I think 1 has to be yes or else the API becomes inconsistent.

As a result, I think the following API would be ideal:

{ foo: 'bar' } matches <Foo foo="bar" />
{ user: { id: 1 } } matches <Foo user={{ id: 1, name: 'Bob' }} />
{ foo: 'bar', qoo: 'qux' } matches <Foo foo="bar" qoo="qux" />
{ style: { width: 20 } } matches <Foo style={{ width: 20, height: 20 }} />
{ style: { width: 20 } } matches <div style={[{ width: 20 }, { height: 20 }]} />
{ foo: 'bar', qoo: 'qux' } does not match <Foo foo="bar" />
{ style: { width: 20 } } does not match <Foo style={[{ width: 20 }, { height: 20 }]} />

Let me know if any of you guys disagree, but I feel like that last rule is probably the right behavior. Basically, I'm thinking we will only "flatten" styles when the node type is a string (ie, it's a DOM node).

This could end up kind of annoying though, because some people could have a test set up where their render method somewhere along the lines has something like:

<input style={[styles.input, error && styles.inputError]} ... />

Then they have a test somewhere that asserts that the inputError styles are applied to the input. With the proposed API above, this works swimmingly BUT... at some point there might be a refactor like the following:

<Input style={[styles.input, error && styles.inputError]} ... />

At this point, this test will end up failing (assuming a shallow render), but its still behaving properly. Now writing the same test becomes a lot more difficult.

I guess the question comes down to whether or not we want to treat the style prop as a special case for all components, or just for DOM components where we know that they behave in this way? I think there are arguments for both cases...

ljharb · 2016-01-17T18:24:39Z

If it matches subsets all the way down, how would I go about not matching a subset, ie, only doing strict matching?

I think that the only special cases should ever be on DOM components - style is just a random prop on anything else.

hartzis · 2016-01-17T19:08:45Z

I think that the only special cases should ever be on DOM components - style is just a random prop on anything else.

I agree with this. @ljharb. I also haven't thought about strict matching. Maybe hold off till users are asking for it? Then maybe an options object passed as second argument with the selector?

@lelandrichardson I'm having trouble understanding <div style={[{ width: 20 }, { height: 20 }]} />. This doesn't seem to currently be a thing in v0.14, it looks like it might be something someday?

But the array for style is currently working for React Native Components, but they have to be Components <Text/> not dom elements/nodes <div/>. And then the trouble is in 'regular react' passing that array for style just becomes a prop of style whose value is an array.

example fiddle https://jsfiddle.net/n38pwsdc/

So i could see this:

{ style: { width: 20 } } matches <Foo style={[{ width: 20 }, { height: 20 }]} />
{ style: { width: 20 } } does not match <div style={[{ width: 20 }, { height: 20 }]} />

but even this is weird because the top one truly only works for react native.

If we just keep it as is just looking for subsets of props, then the user has to know what to expect since they are testing. No flattening needed on our end.

lelandrichardson · 2016-01-17T19:31:48Z

LOL. @hartzis you're completely right. I've just been working way too much in react native recently... we don't need to do anything specific for style then at this point.

As far as strict matching goes... I agree that maybe we just wait on this. As a result, I think the current state of this PR is what we all agree on?

hartzis · 2016-01-18T01:38:31Z

no worries. let me know about those errors and i'll get it updated asap to get this merged in

blainekasten · 2016-01-18T15:02:03Z

LGTM.

lelandrichardson · 2016-01-20T22:10:57Z

LGTM.

[semver-minor] add support for object property selector

blainekasten reviewed Jan 11, 2016
View reviewed changes

hartzis force-pushed the master branch from b76511e to eb9dd29 Compare January 12, 2016 00:54

blainekasten reviewed Jan 12, 2016
View reviewed changes

hartzis force-pushed the master branch from eb9dd29 to b812be0 Compare January 17, 2016 15:44

hartzis reviewed Jan 17, 2016
View reviewed changes

lelandrichardson mentioned this pull request Jan 17, 2016

Add equals method to ShallowWrapper. #124

Merged

add support for object property selector

443a73e

hartzis force-pushed the master branch from b812be0 to 443a73e Compare January 18, 2016 04:38

ljharb added a commit that referenced this pull request Jan 20, 2016

Merge pull request #110 from hartzis/master

3feb0ca

[semver-minor] add support for object property selector

ljharb merged commit 3feb0ca into enzymejs:master Jan 20, 2016

lelandrichardson added the semver: minor New stuff. label Feb 2, 2016

This was referenced Sep 4, 2018

Update enzyme-adapter-react-16 in / from 1.0.1 to 1.4.0 pcardune/redux-firebase-mirror#211

Open

Update 2 dependencies from npm JetBrains/ring-ui#391

Merged

Update 4 dependencies from npm JetBrains/ring-ui#392

Merged

This was referenced Oct 1, 2018

Update enzyme-adapter-react-16 in /modules/frontend from 1.1.1 to 1.5.0 project-accelerate/accelerate#263

Open

Update enzyme-adapter-react-16 in / from 1.1.1 to 1.5.0 leonardofaria/leozera-ui#233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for object property selector #110

add support for object property selector #110

hartzis commented Jan 11, 2016

blainekasten Jan 11, 2016

hartzis Jan 11, 2016

hartzis commented Jan 12, 2016

blainekasten Jan 12, 2016

ljharb Jan 12, 2016

blainekasten Jan 12, 2016

ljharb Jan 12, 2016

blainekasten Jan 12, 2016

ljharb Jan 13, 2016

lelandrichardson Jan 14, 2016

blainekasten Jan 14, 2016

lelandrichardson Jan 14, 2016

hartzis Jan 15, 2016

lelandrichardson commented Jan 15, 2016

hartzis commented Jan 15, 2016

ljharb commented Jan 15, 2016

hartzis commented Jan 17, 2016

hartzis Jan 17, 2016

lelandrichardson commented Jan 17, 2016

ljharb commented Jan 17, 2016

hartzis commented Jan 17, 2016

lelandrichardson commented Jan 17, 2016

hartzis commented Jan 18, 2016

blainekasten commented Jan 18, 2016

lelandrichardson commented Jan 20, 2016

add support for object property selector #110

add support for object property selector #110

Conversation

hartzis commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartzis commented Jan 12, 2016

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

Choose a reason for hiding this comment

lelandrichardson commented Jan 15, 2016

hartzis commented Jan 15, 2016

ljharb commented Jan 15, 2016

hartzis commented Jan 17, 2016

Choose a reason for hiding this comment

lelandrichardson commented Jan 17, 2016

ljharb commented Jan 17, 2016

hartzis commented Jan 17, 2016

lelandrichardson commented Jan 17, 2016

hartzis commented Jan 18, 2016

blainekasten commented Jan 18, 2016

lelandrichardson commented Jan 20, 2016