Support all attribute selector operators #1157

aweary · 2017-09-26T21:02:52Z

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.

aweary · 2017-09-26T21:09:57Z

packages/enzyme/src/Utils.js

@@ -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) {


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

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

ljharb

Awesome!

ljharb · 2017-09-27T03:13:35Z

packages/enzyme-test-suite/test/selector-spec.jsx

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


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

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

fair; beforeAll tends to cause test flakiness in my experience

ljharb · 2017-09-27T03:14:39Z

packages/enzyme/src/Utils.js

@@ -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) {


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

ljharb · 2017-09-27T03:14:53Z

packages/enzyme/src/selectors.js

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


please use the has module for this

ljharb · 2017-09-27T03:15:43Z

packages/enzyme/src/selectors.js

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


can we use slice instead of substr here?

ljharb · 2017-09-27T03:15:47Z

packages/enzyme/src/selectors.js

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


aweary · 2017-09-27T20:41:25Z

@ljharb feedback has been addressed 👌

ljharb

LGTM!

jack-lewin · 2017-09-30T07:30:42Z

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 🙂

jack-lewin · 2017-09-30T07:30:54Z

packages/enzyme-test-suite/test/selector-spec.jsx

+        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);


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

@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 =

jack-lewin · 2017-09-30T07:31:05Z

packages/enzyme-test-suite/test/selector-spec.jsx

+        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);


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?

I think so, thanks!

jack-lewin · 2017-09-30T07:31:09Z

packages/enzyme/src/selectors.js

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


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?

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.

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

lelandrichardson · 2017-09-30T20:48:12Z

packages/enzyme/src/selectors.js

@@ -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 = '|=';


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?

@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?

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

ljharb · 2017-11-08T18:56:44Z

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

ljharb

LGTM

ljharb · 2017-11-10T19:00:45Z

packages/enzyme/src/selectors.js

+      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 


this trailing space is breaking the linter

Woops! Fixed 👌

ljharb · 2017-11-11T06:04:55Z

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

nfcampos

LGTM

Brandon Dail added 5 commits September 26, 2017 13:32

Test attribute value selectors with public API

b88114b

Support all attribute value selector operators

4c19481

Don't attempt to match non-strings in fuzzy attribute selectors

de3069d

Fix suffix attribute selector logic

11663db

Add tests for all attribute selector operators

94011e2

aweary requested review from ljharb and lelandrichardson September 26, 2017 21:02

aweary commented Sep 26, 2017

View reviewed changes

ljharb requested changes Sep 27, 2017

View reviewed changes

Brandon Dail added 3 commits September 27, 2017 13:13

Use has module instead of hasOwnProperty

9751ed8

Inline nodeHasMatchingProperty logic in selector module

6573ebe

Use slice instead of substr for suffix/prefix

ead8641

aweary force-pushed the add-attribute-operators branch from a9c3776 to bb08320 Compare September 27, 2017 20:39

ljharb approved these changes Sep 27, 2017

View reviewed changes

ljharb added semver: minor New stuff. API: mount API: shallow labels Sep 27, 2017

aweary force-pushed the add-attribute-operators branch from bb08320 to c64dfe0 Compare September 29, 2017 19:10

jack-lewin reviewed Sep 30, 2017

View reviewed changes

lelandrichardson reviewed Sep 30, 2017

View reviewed changes

aweary mentioned this pull request Nov 8, 2017

CSS Attribute selector should support |= and ~= #620

Closed

aweary self-assigned this Nov 8, 2017

aweary force-pushed the add-attribute-operators branch from c64dfe0 to 9d9a3a8 Compare November 10, 2017 17:52

Fix duplicate Infinity test

d64abf3

Brandon Dail added 2 commits November 10, 2017 09:56

Fix "HYPEN" typo

45a1a7d

Remove nodeHasProperty tests

e8dd8d9

ljharb approved these changes Nov 10, 2017

View reviewed changes

ljharb reviewed Nov 10, 2017

View reviewed changes

Remove trailing space

dec1505

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

nfcampos approved these changes Nov 11, 2017

View reviewed changes

ljharb force-pushed the add-attribute-operators branch from 8340c6b to dec1505 Compare November 11, 2017 18:40

ljharb merged commit f0fc012 into enzymejs:master Nov 11, 2017

This was referenced Nov 15, 2017

Update enzyme in / from 3.1.1 to 3.2.0 kornicameister/korni#141

Merged

Update 5 dependencies from npm storybookjs/storybook#2312

Merged

dependencies bot mentioned this pull request Dec 5, 2017

Update 23 dependencies from npm JetBrains/ring-ui#31

Merged

dependencies bot mentioned this pull request Mar 1, 2018

Update enzyme in / from 3.1.0 to 3.3.0 pcardune/redux-firebase-mirror#15

Open

ljharb added a commit that referenced this pull request Jul 8, 2018

RSTTraversal: remove nodeHasProperty export; broken since #1157

4fe4961

ljharb added a commit that referenced this pull request Jul 8, 2018

RSTTraversal: remove nodeHasProperty export; broken since #1157

edabb1b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support all attribute selector operators #1157

Support all attribute selector operators #1157

aweary commented Sep 26, 2017

aweary Sep 26, 2017

ljharb Sep 27, 2017

ljharb left a comment

ljharb Sep 27, 2017

aweary Sep 27, 2017

ljharb Sep 27, 2017

ljharb Sep 27, 2017

ljharb Sep 27, 2017

ljharb Sep 27, 2017

ljharb Sep 27, 2017

aweary commented Sep 27, 2017

ljharb left a comment

jack-lewin commented Sep 30, 2017

jack-lewin Sep 30, 2017

aweary Oct 2, 2017

jack-lewin Sep 30, 2017

aweary Oct 2, 2017

jack-lewin Sep 30, 2017

aweary Oct 2, 2017

jack-lewin Oct 2, 2017

lelandrichardson Sep 30, 2017

aweary Oct 2, 2017

ljharb Oct 2, 2017

ljharb commented Nov 8, 2017

ljharb left a comment

ljharb Nov 10, 2017

aweary Nov 10, 2017

ljharb commented Nov 11, 2017

nfcampos left a comment

Support all attribute selector operators #1157

Support all attribute selector operators #1157

Conversation

aweary commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a 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

aweary commented Sep 27, 2017

ljharb left a comment

Choose a reason for hiding this comment

jack-lewin commented Sep 30, 2017

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

ljharb commented Nov 8, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Nov 11, 2017

nfcampos left a comment

Choose a reason for hiding this comment