fixes #590: change isCompoundSelector to not match prop selector #595
Conversation
joeduncan
force-pushed
the
fix-590
branch
2 times, most recently
from
b050472
to
fee821e
Compare
|
Thanks for this @joeduncan, all the extra tests are awesome. Since all of our existing tests look good as well, this LGTM. I'd like to keep this open for review since I'm not 100% with regexes, so maybe someone like @ljharb can take a look as well. |
| @@ -189,7 +189,7 @@ export function selectorError(selector, type = '') { | |||
| ); | |||
| } | |||
|
|
|||
| export const isCompoundSelector = /([a-z]\.[a-z]|[a-z]\[.*\]|[a-z]#[a-z])/i; | |||
| export const isCompoundSelector = /^[\.#]?-?[_a-z]+[_a-z0-9-]*[\.\[#]/i; | |||
There was a problem hiding this comment.
i wish we had commented regexps for situations like this. Let's see:
/
^ # start
[\.#]? # a dot or a hash, 0 or 1
-? # a hyphen, 0 or 1
[_a-z]+ # any letter or underscore, 1 or more
[_a-z0-9-]* # any letter, underscore, number, or hyphen, 0 or more
[\.\[#] # any dot, left bracket, or hash
/iEven after that I'm not really sure how to reason out what it's matching against. I wonder if we could write this as multiple different regexes, instead of trying to combine it into one case?
There was a problem hiding this comment.
Even after that I'm not really sure how to reason out what it's matching against. I wonder if we could write this as multiple different regexes, instead of trying to combine it into one case?
Agreed, that's the approach I took with #591 because using a single regex was just too confusing. Breaking this into multiple regexes seems reasonable, but at a certain point it feels like we'd be implementing our own parser and I wonder if it would be better to just put the work into making #534.
There was a problem hiding this comment.
[\.#]? # anything but a hash, 0 or 1
not sure it matters here but I think this one is actually '. or a #, 0 or 1'
There was a problem hiding this comment.
Whoops, you're right. Will update my comment
There was a problem hiding this comment.
Actually you'll need a parser because CSS selectors are described using a LL(1) grammar and therefore cannot be parsed by a regular expression.
But you don't need to reinvent the wheel - use an existing selector engine like Sizzle for example and kill all the birds.. eh bugs with one stone.
There was a problem hiding this comment.
We could bring in xregexp if we wanted to make this regexp more readable with comments. I generally think that regexps should have comments, but I honestly don't find this one to be too bad.
Also, inside of brackets, I don't think you need to escape things like . and [, so you could remove a couple of backslashes to clean it up a bit.
Overall, this seems to be an improvement so I think we might as well merge it in. In the long run, if we want to support the full range of CSS selectors, I agree with @Jazen that we should bring in a parser.
|
Is there anything I can do here? |
|
@joeduncan Thanks for your contribution! When you have a moment, will you please rebase this onto the latest master branch? |
|
Rebased and ready for merge. |
No description provided.