Use postcss-selector-parser for selector handling #458

jacobp100 · 2016-06-16T08:22:53Z

No description provided.

aweary · 2016-06-16T14:28:01Z

Very nice, I was just mentioning doing something like this here. Do you know if this would also resolve #456?

@ljharb what do you think?

ljharb · 2016-06-16T14:33:31Z

src/ComplexSelector.js

      return list;
    }, []);
  }

  handleSelectors(selectors, wrapper) {
    const recurseSelector = (offset, fn, pre) => {
-      const predicate = pre || this.buildPredicate(selectors[offset]);
+      const predicate = pre || this.buildPredicate(selectors[offset].toString());


toString is not guaranteed to return a String - please wrap in String() instead, throughout.

ljharb · 2016-06-16T14:39:23Z

Made a few comments - overall I think this looks awesome! @jacobp100, could you add more shallow and mount tests that cover a number of expected-to-work selectors, so that things don't break without us knowing in the future?

aweary · 2016-06-16T14:41:24Z

@jacobp100 I mentioned a couple of valid selectors that are broken in this comment. If we could test those as well that'd be great.

blainekasten · 2016-06-16T15:15:05Z

src/ShallowTraversal.js

+  const nodes = getAst(selector).nodes[0];
+  return AND(nodes.map(node => {
+    switch (node.type) {
+      case 'class':


It's a bit nit, but the old way of using object constants seemed safer/nicer than just strings. Can you update this to use constants?

The strings come from postcss-selector-parser, and aren't exported. But I could definitely put a map in util if you think that's best!

Yeah. Just put it in a map so there is a central place of seeing the node types supported

It would also be great to file an issue or send a PR upstream to get postcss-selector-parser to export the strings.

blainekasten · 2016-06-16T15:17:13Z

This is awesome and exciting. Thanks for doing this work @jacobp100

ljharb · 2016-06-16T16:01:20Z

This LGTM! @lelandrichardson, any final thoughts before we merge?

aweary · 2016-06-16T16:14:58Z

For reference, I've opened a PR upstream to export the selector types: postcss/postcss-selector-parser#70

lelandrichardson · 2016-06-16T16:23:23Z

Really liking this direction.... Thanks for this work @jacobp100 . I have a couple of questions though:

I gather we are ignoring combined selectors (ie, .foo, .bar) in some situations (or is it all situations?). In this case, we should probably throw a warning or something to the developer explaining that only the first one is being used, or else it might be misleading.
are we no longer warning at all for unsupported CSS selectors? It seems there are still a number left.
this gets an AST for the CSS selectors (which is great!) but leaves traversal up to us. For descendant selectors etc. this can be quite hairy. I don't seem to see a lot of tests covering this behavior? It seems important to me if we are adding descendant (and other nonlocal selectors like sibling, direct descendant, etc.) then we will really want to make sure we get this right.

Sorry if I'm missing something... but I feel like this needs to be merged in with a little bit more care. I love the direction and it seems like this will be much more robust than attempting the parsing ourselves, but let's make sure we understand the change we are making, and ensure that any functionality that we are adding ends up working as close to the CSS spec as it can be, or if it differs, documenting the differences.

Does that seem reasonable?

jacobp100 · 2016-06-16T16:32:21Z

Agreed. I think it would make sense to throw for multiple selectors.

I think I throw for all invalid selectors. Can you see a case where I haven't?

I'm actually looking into traversals in another branch. It's a bigger undertaking though.

aweary · 2016-06-16T16:37:19Z

One concern I have is bringing in a dependency that isn't being actively maintained. Looking at postcss-selector-parser it doesn't seem like they are actively maintaining it at the moment. The last commit was almost a month ago and there are PRs that haven't been addressed for about the same amount of time.

If we make this a dependency and run into upstream issues we may be facing an uphill battle getting those fixed. They could just be busy right now, but it's worth considering.

edit: coming back to this a few months later, this was dumb 👎 few commits !== unmaintained.

ljharb · 2016-06-16T16:41:16Z

If it's abandoned that's one thing, but we can always fork it and publish our own in that case - I'm not too concerned about anything that's in a git repo.

lelandrichardson · 2016-06-16T16:43:29Z

An alternative that's been brought up before is this: https://github.com/jquense/bill which is used in https://github.com/jquense/teaspoon

cc @jquense to see if he has anything to add on this thread!

jquense · 2016-06-16T17:06:20Z

I've suggested bill before as a possibility. i'd be happy to have it consumed by more testing libraries, I'd also be happy to have more collaborators which may be a better option to forking something :). bill solid at this point and supports back to react13

aweary · 2016-10-18T15:22:57Z

@jquense coming back to this, it looks like bill would take care of traversal for us right? I would be open to trying to implement this with bill instead so we don't have to include a bunch of AST traversal logic here too.

jquense · 2016-10-18T15:43:42Z

@aweary ya bill, exposes a bunch of traversal utilities, and abstracts out the differences between element and instance trees so that you can use the same traversal logic (where that makes sense) for both. High level it wraps matched trees in Node objects that expose DOM Element-esque traversal properties prevSibling, children, parentNode, etc; it makes implementing a wide range of selectors nice and easy

feel free to ping me where ever if you have any questions/concerns 👍

aweary · 2017-02-15T22:12:08Z

@jacobp100 is there any chance you still want to work on this?

jacobp100 · 2017-02-16T07:34:30Z

I think this pr should be complete. But let me know if you have any bugs!

ljharb · 2017-02-16T08:06:35Z

@jacobp100 at the least it'd need a fresh rebase - do you mind doing that?

aweary · 2017-04-09T21:27:41Z

@jacobp100 would you mind rebasing and resolving the merge conflicts? If not, no worries. I can try to take this over if you don't have the time. I'd really like to get this merged soon.

jquense · 2017-04-09T21:53:27Z

@aweary you are everywhere man

aweary · 2017-04-09T22:11:08Z

@jquense so many interesting projects, so little time 😄

jquense · 2017-04-10T12:31:49Z

@aweary I know! any shot of y'all incorporating https://github.com/jquense/bill ? id love to not maintain my other testing library on top of it and just use enzyme :P

aweary · 2017-08-31T18:54:36Z

@jacobp100 At this point Enzyme has been entirely rewritten and this PR has fallen really far out of sync. I'm going to close this in favor of #1086 which uses a custom selector parser that supports the variant selector grammar we expect and utilizes the new RST spec we've defined.

Sorry this never got merged, we really appreciate all the hard work you did on this!

Jacob Parker added 2 commits June 16, 2016 09:22

Use postcss-selector-parser for selector handling

a745716

Validate attribute names

48f69e4

ljharb reviewed Jun 16, 2016
View reviewed changes

blainekasten reviewed Jun 16, 2016
View reviewed changes

Add more tests for selector parsing

709bf09

ljharb mentioned this pull request Jun 16, 2016

Fix falsy attributes being ignored #459

Closed

ljharb added enhancement API: shallow API: mount semver: minor New stuff. labels Jun 16, 2016

aweary mentioned this pull request Jun 16, 2016

Use and export type constants for selectors postcss/postcss-selector-parser#70

Merged

aweary mentioned this pull request Jun 16, 2016

add ":not" to negate css prop selector #460

Closed

aweary mentioned this pull request Jul 11, 2016

Attribute selector values containing # broke in 2.4.0 #495

Closed

aweary mentioned this pull request Sep 9, 2016

is(':checked') not working to inspect checked state #585

Closed

aweary mentioned this pull request Oct 11, 2016

fixes #590: change isCompoundSelector to not match prop selector #595

Merged

aweary self-assigned this Apr 9, 2017

aweary closed this Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use postcss-selector-parser for selector handling #458

Use postcss-selector-parser for selector handling #458

jacobp100 commented Jun 16, 2016

aweary commented Jun 16, 2016 •

edited

Loading

ljharb Jun 16, 2016

ljharb commented Jun 16, 2016

aweary commented Jun 16, 2016

blainekasten Jun 16, 2016

jacobp100 Jun 16, 2016

blainekasten Jun 16, 2016

ljharb Jun 16, 2016

blainekasten commented Jun 16, 2016

ljharb commented Jun 16, 2016

aweary commented Jun 16, 2016

lelandrichardson commented Jun 16, 2016

jacobp100 commented Jun 16, 2016

aweary commented Jun 16, 2016 •

edited

Loading

ljharb commented Jun 16, 2016

lelandrichardson commented Jun 16, 2016

jquense commented Jun 16, 2016

aweary commented Oct 18, 2016

jquense commented Oct 18, 2016

aweary commented Feb 15, 2017

jacobp100 commented Feb 16, 2017

ljharb commented Feb 16, 2017

aweary commented Apr 9, 2017

jquense commented Apr 9, 2017

aweary commented Apr 9, 2017

jquense commented Apr 10, 2017

aweary commented Aug 31, 2017

Use postcss-selector-parser for selector handling #458

Use postcss-selector-parser for selector handling #458

Conversation

jacobp100 commented Jun 16, 2016

aweary commented Jun 16, 2016 • edited Loading

ljharb Jun 16, 2016

Choose a reason for hiding this comment

ljharb commented Jun 16, 2016

aweary commented Jun 16, 2016

blainekasten Jun 16, 2016

Choose a reason for hiding this comment

jacobp100 Jun 16, 2016

Choose a reason for hiding this comment

blainekasten Jun 16, 2016

Choose a reason for hiding this comment

ljharb Jun 16, 2016

Choose a reason for hiding this comment

blainekasten commented Jun 16, 2016

ljharb commented Jun 16, 2016

aweary commented Jun 16, 2016

lelandrichardson commented Jun 16, 2016

jacobp100 commented Jun 16, 2016

aweary commented Jun 16, 2016 • edited Loading

ljharb commented Jun 16, 2016

lelandrichardson commented Jun 16, 2016

jquense commented Jun 16, 2016

aweary commented Oct 18, 2016

jquense commented Oct 18, 2016

aweary commented Feb 15, 2017

jacobp100 commented Feb 16, 2017

ljharb commented Feb 16, 2017

aweary commented Apr 9, 2017

jquense commented Apr 9, 2017

aweary commented Apr 9, 2017

jquense commented Apr 10, 2017

aweary commented Aug 31, 2017

aweary commented Jun 16, 2016 •

edited

Loading

aweary commented Jun 16, 2016 •

edited

Loading