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

Support Property Selector #70

Merged
merged 1 commit into from Jan 10, 2016

Conversation

Projects
None yet
7 participants
@blainekasten
Copy link
Contributor

blainekasten commented Dec 12, 2015

Per discussion in #4 and continued discussion here. This PR adds support for querying react properties, and not html attributes. The following syntax works:

  • component.find('input[type="text"][value="1"])
  • component.find('label[htmlFor="button"]')
  • component.find('div[data-wrapper]')
@@ -128,3 +136,46 @@ export function AND(fns) {
return true;
};
}

export const attributePropMap = {

This comment has been minimized.

Copy link
@goatslacker

goatslacker Dec 12, 2015

Member

curious is there an npm module we can extract these from?

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 12, 2015

Author Contributor

I did find one. The only potential problem is that react supports some non-standard attributes, which aren't covered in the package I found, and likely wouldn't be covered by other packages.

This comment has been minimized.

Copy link
@smacker

smacker Dec 18, 2015

Contributor

how about take it from react itself? You can import HTMLDOMPropertyConfig and SVGDOMPropertyConfig from renderers/dom/shared. I think it's better, because when react team add new property it starts working here without any changes.

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 18, 2015

Author Contributor

This will be going away entirely actually.

@blainekasten blainekasten changed the title WIP: Attribute Selector Discussion: Attribute Selector Dec 12, 2015

- attr syntax(`[foo="bar"]`, `[foo]`, etc.);

A note on attribute selectors: For compatibilty, you can use either the
react prop name, or the native html selector. For example, the following

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 12, 2015

Member

I still see this as a horribly bad API decision. [] means "HTML attributes", not "React component props" - the two aren't the same thing. Wouldn't the code be a lot simpler to use a different syntax instead of smashing two things into one existing syntax?

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 12, 2015

Member

In addition, this is a decision that will be almost impossible to unmake - whereas introducing a different syntax now won't in any way prevent bloating the attribute selector syntax later (and just using the same code) if that's what we decide we want.

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 12, 2015

Author Contributor

cc @lelandrichardson

I'm open for changing either way.
@ljharb what about the context of different rendering layers. Think ReactNative, ReactHardware. In those contexts, do we just say the [] isn't for them? I think leland had a good point that the [] selector was built for XML, not explicitly the DOM. so in a ReactNative/Hardware standpoint, the Props are the attributes of the "rendered" xml

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 12, 2015

Member

as I understand it (@lelandrichardson can confirm) enzyme was originally built to replicate much of jQuery's API, which is why the querySelectorAll syntax is used for the selector string. If we're going to be deviating from it, for React Native or otherwise, that's a bigger discussion - does React Native itself use selector syntax?

This comment has been minimized.

Copy link
@hartzis

hartzis Dec 14, 2015

Contributor

@ljharb I can understand your hesitation with the API decision. We solved something similar in the testing library skin-deep with tree.subTreeLike('*', {foo: 'bar'}).

We wanted to query for specific components using props, so we could use specific testing properties like data-testref='foo' that didn't interfere with our ui teams use of classnames.

cc @lelandrichardson @blainekasten

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 14, 2015

Author Contributor

I started on an implementation to use seperate syntax ({} for props, [] for html attr). It seems feasible. In shallow renders, I'm basically faking it by mapping the html attr selector to the matching react prop since I don't get an actual render. I'm just waiting on word from @lelandrichardson if that is the way we want this togo.

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 15, 2015

Author Contributor

I just pushed up the changes that @ljharb requested. Hopefully this can help the conversation move along.

looking for feedback from @lelandrichardson and @ljharb

instProps = node._store.props;
} else {
instProps = node.props;
}

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 13, 2015

Collaborator

there should be a propsOfNode() function that abstracts this away... it's also important to check for falsy nodes as well where this would probably throw.

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 14, 2015

Author Contributor

👍

@blainekasten blainekasten changed the title Discussion: Attribute Selector Discussion: Attribute/ReactProp Selector Dec 15, 2015

return inst => instHasId(inst, selector.substr(1));
case SELECTOR.ATTR_TYPE:
const attrKey = selector.split(/\[([a-z\-\:]*?)(=|\])/)[1];

This comment has been minimized.

Copy link
@0xR

0xR Dec 16, 2015

are we sure attributes can't have capitals?

if not I would expect some useful error message

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 16, 2015

Author Contributor

Good catch. That is actually a bug now that I think about it. svg introduced a lot of camelCase attributes, like preserveAspectRatio


return node => instHasAttribute(node, attrKey, attrValue);
case SELECTOR.PROP_TYPE:
const propKey = selector.split(/\{([a-zA-Z]*?)(=|\})/)[1];

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 16, 2015

Author Contributor

this should probably also include -'s for data-* props

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 16, 2015

wow, thanks for all this work! I'm going to try and take a closer look tonight after we finish up this hackathon. Looks great!

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

Hey @blainekasten ,

I hate to give you more work to do... but i was hoping you could change this back to the original [prop] syntax. @ljharb and I discussed the pros/cons of each syntax and we agreed that handling just [prop] and getting rid of {prop} syntax.

To recap, the syntax we would like is:

  • [prop="foo"] gets nodes with prop="foo"
  • [foo] gets nodes that have a foo prop defined
  • there will be no special mapping between props and html attributes such as htmlFor => for.
@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Dec 18, 2015

No worries. I want to see this land. I can do that. I agree with that API also honestly. Glad to have it sorted out. I'll finish it tomorrow 👍

@blainekasten blainekasten changed the title Discussion: Attribute/ReactProp Selector Support Attribute Selector Dec 18, 2015

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

wow, good work on the tests! Istanbul is becoming a bit of a pain since we are using babel... I need to figure out how to get istanbul working with the original source lines so that it makes more sense. just ignore for now if the percentage goes down.

I'm curious of a couple of things:

  1. What's the behavior of numeric proptypes? Seems like something that we could also use the string selector for (or not?). Either way, the expectations should be tested.
const wrapper = shallow(<div><Foo bar={2} /></div>);
expect(wrapper.find('[bar="2"]')).to.have.length(1);

Does the above pass? (I'm not necessarily saying it should)

@@ -116,7 +116,26 @@ export function selectorError(selector) {
);
}

export const isCompoundSelector = /[a-z]\.[a-z]/i;
export const isCompoundSelector = /([a-z]\.[a-z]|[a-z]\[.*\])/i;

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

will this always work? what about [foo][bar]?

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 18, 2015

Author Contributor

It does, I tested just tested that exact scenario, but also this test effectively proves it also, https://github.com/airbnb/enzyme/pull/70/files#diff-e08e795cdfa46e03ea97df132b57d75aR167

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

My testing shows that it doesnt? div[foo][bar] matches but [foo][bar] does not

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 21, 2015

Author Contributor

Maybe test again? I literally just tested both those ideas and they both matched.

const onChange = () => {};
const wrapper = mount(
<div>
<span htmlFor="foo" onChange={onChange}preserveAspectRatio="xMaxYMax" />

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

missing whitespace?

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

Also, what is the expected behavior for falsy but present props?

  • does <div foo="" /> match [foo]?
  • does <Foo visible={false} /> match [visible]?
const nodeProps = propsOfNode(node);

if (propValue) {
return nodeProps[propKey] === propValue;

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

i guess my questions boil down to: should this be == or ===?

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 18, 2015

Author Contributor

That doesn't work for situations like 'false' or 'true'

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

yah... so we may need some special case logic put in here. let's try and figure out what the corner cases are and what the desired behavior is...

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 18, 2015

Member

===. A prop of 3 and a prop of '3' are different and shouldn't be selected in the same group.

return nodeProps[propKey] === propValue;
}

return nodeProps.hasOwnProperty(propKey);

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

and, should this be nodeProps.hasOwnProperty(propKey) or !!nodeProps[propKey]

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 18, 2015

Author Contributor

I haven't ran into a situation where hasOwnProperty didn't work. Can you think of a test case?

This comment has been minimized.

Copy link
@blainekasten

blainekasten Dec 18, 2015

Author Contributor

In fact, I just tested this with the new changes and it doesn't work in this situation.

wrapper = <div foo={false} />

wrapper.find('[foo=false]')

In that situation that turns into

!!wrapper['foo'] // false

which then says it doesn't have the prop, though it does. So hasOwnProperty seems to be the way to go.

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Dec 18, 2015

Collaborator

this is basically the foo={false} question. If it has a foo property but it's set to something falsy, do we still want to return true?

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Dec 18, 2015

Ahh good catch. I didn't think about non-string values.. I'll have to work on this a bit more.

Empty string currently works fine:

<div foo="" /> matches [foo] or [foo=""]
<Foo visible={false} /> DOES match [visible], not sure if that's the right call or not?

I guess in the situation of non string prop values, I would think dropping the quotes would be the way to do it.

So <Foo visible={false} /> could match [visible=false] but not [visible="false"]

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

@ljharb what do you think? How should empty values be handled? should [foo] match <div foo="" />?

I'm trying to think pragmatically, and it seems to me that if i look for [visible], I do NOT want <Foo visible={false} /> to match...

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Dec 18, 2015

If we want to stick to querySelector syntax, a query of [foo] only looks for a node that has that attribute, irrelevant of the value.

Some new code that I have you could do this.

[visible=false] if you want the non-visible things.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 18, 2015

That's a very good question - since this is a string-based API, it will already only work for a subset of prop values (and won't work for Symbol props). I think we can just draw a line and say that for this limited form of findWhere/filterWhere, [foo] should not match falsy values, and [foo=${bar}] should mean that a bar of a non-string value should never match anything.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 18, 2015

Alternatively, I'd be totally fine with saying that [foo] matches any non-undefined value - and that might be more in line with people's expectations.

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

I think I like [foo] matching non-undefined-values.

That just leaves the non-string-based values... in this case we could say that things have to be === to each other... (so [foo="2"] would NOT match <Foo foo={2} />).

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Dec 18, 2015

agreed. I think non-string values is for xWhere to handle.

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

Let's also add two tests describing its behavior for <Foo ref="foo" /> and <Foo key={1} /> since those are not treated the same as other props in react...

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Dec 18, 2015

Feeling a little lost by the string of comments. Can you drop me a few examples of what tests you'd want to pass/fail?

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Dec 18, 2015

I don't think adding support for ref makes much sense since React is moving away from the string ref api. It's almost guaranteed that in the coming months/year that react won't support a string for ref and only accept a fn.

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Dec 18, 2015

Agreed. I don't want to support it... I just want to add a test that asserts that it's not supported so we know that that doesn't change at some point

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Jan 5, 2016

@blainekasten thanks for bearing with us for this whole conversation!

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 5, 2016

No prob! It's a pretty big api addition. Always gotta make sure it serves the right direction.

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 5, 2016

@lelandrichardson This is ready to go now! Thanks

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Jan 7, 2016

@blainekasten one last request: can you squash your commits before I merge?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 7, 2016

@blainekasten a rebase would be preferred over a merge

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 7, 2016

Yeah. I'm trying to rebase. I always struggle through the squashing process...

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 7, 2016

sadly github doesn't make it easy with a rebase button. on your branch, git fetch source; git rebase source/master where source is the git remote for this repo should do it tho?

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 7, 2016

Yeah.. it is too hard. I am not following exactly for that suggestion.

I tried git rebase -i HEAD~n and squashed the previous commits.. but it didn't squash them haha

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 7, 2016

Alternatively, you could make a brand new branch locally, recreate it manually by checking things out or cherry picking from the original branch, and then, force push the new local branch to the original remote branch, which will update the PR.

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Jan 7, 2016

on your branch, figure out the total # of commits n that you have that you want to squash. Run git rebase -i HEAD~n. In the menu, leave the first commit as pick <sha> <message>. For all of the following commits, change it to squash <sha> <message>. Submit that screen (:wq if you're using vim), then in the next screen submit that screen as well, unless you want to change the commit message, which you can do.

After that, run git push -f

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 7, 2016

Are you guys happy enough with 2 squashedish commits? haha. I can fix it if you want. I just get so worried when I do this that I'll break something.

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 7, 2016

Oh boom. I got this down now. Alright, I think we're good to :)


it('returns a boolean if passed a stringified bool', () => {
expect(coercePropValue('true')).to.be.true;
expect(coercePropValue('false')).to.be.false;

This comment has been minimized.

Copy link
@lelandrichardson

lelandrichardson Jan 7, 2016

Collaborator

whoops. something got merged into master that prevents these types of tests. You need to change these to .to.equal(true) instead. Our linter prevents it the use of these now. You can run npm run lint to ensure everything is fixed.

This comment has been minimized.

Copy link
@blainekasten

blainekasten Jan 7, 2016

Author Contributor

when I run the linter, those aren't reported. But I changed them, we'll see if this gets it passing

This comment has been minimized.

Copy link
@ljharb

ljharb Jan 7, 2016

Member

@blainekasten you may need to fetch and rebase again - there's been some merges today.

@blainekasten blainekasten changed the title Support Attribute Selector Support Property Selector Jan 8, 2016

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 9, 2016

This should be ready to go now. Thanks

@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Jan 9, 2016

hey @blainekasten it looks like it has some merge conflicts. Some stuff got merged in that must be conflicting. Can you rebase once more and resolve whatever conflicts show up? Thanks!

Add support for the prop selector
cleanup

mounted attr api

Add documentation for attr api

the rest of the attribute mapping and test for colons

react 13/14 compatibility

clean up code by utilizing utils method

fix lint error

Change syntax for react property lookups

fix lint errors

Fix key lookup bug, and simplify regex

Change back to [] syntax for props only

fix documentation

split out test coverage

more test coverage

fix lint issue

support non-string values for prop querying

Add tests for not finding ref or key

string based selector only

deny undefined values

fix lint issues

Revert to 80b14d3

support multiple literal types for property lookups

WIP: Early push for discussion

cleanup

Add support for prop selector

clean up code by utilizing utils method

fix lint error

Change syntax for react property lookups

Change back to [] syntax for props only

split out test coverage

fix lint issue

support non-string values for prop querying

string based selector only

fix lint issues

Revert to 80b14d3

support multiple literal types for property lookups

Revert "support multiple literal types for property lookups"

This reverts commit 72872fa.

undo revert

Trigger new build

add support for prop selector

fix lint errors
@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 10, 2016

Alright. Should be good again. Hopefully it can get merged before something else conflicts again :)

lelandrichardson added a commit that referenced this pull request Jan 10, 2016

Merge pull request #70 from blainekasten/master
Support Property Selector

@lelandrichardson lelandrichardson merged commit 53f43ef into airbnb:master Jan 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 96.581%
Details
@lelandrichardson

This comment has been minimized.

Copy link
Collaborator

lelandrichardson commented Jan 10, 2016

Awesome work. Thank you!

@blainekasten

This comment has been minimized.

Copy link
Contributor Author

blainekasten commented Jan 10, 2016

Thanks for merging! No problem. I'm excited about this project. Thanks for starting it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.