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 all attribute selector operators #1157

Merged
merged 12 commits into from
Nov 11, 2017

Conversation

aweary
Copy link
Collaborator

@aweary aweary commented Sep 26, 2017

  • Adds support for all attribute selector operators
  • Removes nodeHasProperty in favor of nodeHasMatchingProperty
  • Updates property/attribute tests to use the public API instead of importing the internal nodeHasProperty utility. This decouples the tests from the internal implementation and also makes them more robust. I discovered Unable to parse selectors with numeric values in exponential notation #1155 while doing this.

@@ -219,23 +219,17 @@ export function AND(fns) {
return x => fnsReversed.every(fn => fn(x));
}

export function nodeHasProperty(node, propKey, propValue) {
export function nodeHasMatchingProperty(node, propKey, matcher) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could probably just inline this in matchAttributeSelector since its only used once, and its not exported for testing like nodeHasProperty was

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea, the less we export the better

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

describe('selectors', () => {
tests.forEach(({ describeMethod, name, renderMethod }) => {
before(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is "before all"; could we use beforeEach instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but "before all" is the intention here, since expectAttributeMatch only needs to be initialized once per renderMethod

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair; beforeAll tends to cause test flakiness in my experience

@@ -219,23 +219,17 @@ export function AND(fns) {
return x => fnsReversed.every(fn => fn(x));
}

export function nodeHasProperty(node, propKey, propValue) {
export function nodeHasMatchingProperty(node, propKey, matcher) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea, the less we export the better

return nodeHasMatchingProperty(node, token.name, (nodePropValue, nodeProps) => {
const { operator, value } = token;
if (token.type === ATTRIBUTE_PRESENCE) {
return Object.prototype.hasOwnProperty.call(nodeProps, token.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the has module for this

* [type^="image"] matches type="imageobject"
*/
case PREFIX_ATTRIBUTE_OPERATOR:
return value === '' ? false : nodePropValue.substr(0, value.length) === value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use slice instead of substr here?

* [type$="image"] matches type="imageobject"
*/
case SUFFIX_ATTRIBUTE_OPERATOR:
return value === '' ? false : nodePropValue.substr(-value.length) === value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here?

@aweary
Copy link
Collaborator Author

aweary commented Sep 27, 2017

@ljharb feedback has been addressed 👌

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jack-lewin
Copy link
Contributor

Hi @aweary - I've been meaning to put in a PR for this, looks like you beat me to it!

Thought it'd be a good opportunity to see how a more experienced dev went about the same task. Hope you don't mind me commenting on a couple of bits 🙂

expectAttributeMatch(<div rel="foo bar baz" />, '[rel~="baz"]', true);
expectAttributeMatch(<div rel="foo bar baz" />, '[rel~="foo"]', true);
expectAttributeMatch(<div rel="foo bar baz" />, '[rel~="foo bar"]', false);
expectAttributeMatch(<div rel={1} />, '[rel~=1]', false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong - shouldn't this return true? I think ~= should still match if it's exactly equal, with no whitespace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-lewin this implementation only supports matching strings with these partial match operators, so all other types will return false. If you want to match exactly equal values you should use =

expectAttributeMatch(<div data-foo={Infinity} />, '[data-foo=+Infinity]', true);
expectAttributeMatch(<div data-foo={Infinity} />, '[data-foo=-Infinity]', false);
expectAttributeMatch(<div data-foo={Infinity} />, '[data-foo=NaN]', false);
expectAttributeMatch(<div data-foo={0} />, '[data-foo=Infinity]', false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this test is duplicated at the bottom of the it block - should one of them be testing -Infinity, as per the old tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, thanks!

* [rel~="copyright"] matches rel="copyright other"
*/
case WHITELIST_ATTRIBUTE_OPERATOR:
return nodePropValue.split(' ').indexOf(value) !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would throw an error if nodePropValue isn't a string - is it safe to assume that if someone's using ~=, they're testing a string? Or would it be safer to return false if not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an earlier check that tests for non-string values, so we can safely assume that nodePropValue and value are both strings in this case.

is it safe to assume that if someone's using ~=, they're testing a string?

Yupp, that's exactly the assumption! These operators don't make a whole lot of sense with other values like numbers or objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must've missed that. It makes a lot more sense now, thanks for explaining!

@@ -32,6 +34,13 @@ const ATTRIBUTE_VALUE = 'attributeValueSelector';
const PSEUDO_CLASS = 'pseudoClassSelector';
const PSEUDO_ELEMENT = 'pseudoElementSelector';

const EXACT_ATTRIBUTE_OPERATOR = '=';
const WHITELIST_ATTRIBUTE_OPERATOR = '~=';
const HYPEN_ATTRIBUTE_OPERATOR = '|=';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a typo? I wouldn't expect this to be called "hyphen attribute operator", but i have also never heard of "hypen" so i'm pretty sure that's what this is attempting to be called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lelandrichardson Yeah "hypen" is a typo, it should be hyphen. I wasn't really sure what to call it, the spec doesn't provide canonical names and it's kind of a weird operator. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm - it can also be a whole word, not just a hyphenated section. maybe "hyphenated attribute operator" is still the best name tho

@ljharb
Copy link
Member

ljharb commented Nov 8, 2017

@aweary If this can be rebased and existing comments addressed, we can probably re-review it and get it in :-)

@aweary aweary self-assigned this Nov 8, 2017
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return is(nodePropValue, value);
/**
* Represents an element with the att attribute whose value is a whitespace-separated
* list of words, one of which is exactly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this trailing space is breaking the linter

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops! Fixed 👌

@ljharb ljharb requested review from nfcampos, blainekasten and a team November 10, 2017 19:13
@ljharb
Copy link
Member

ljharb commented Nov 11, 2017

I'll merge this tomorrow if there's no objections.

Copy link
Collaborator

@nfcampos nfcampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants