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

Selecting React Components via CSS prop selector #534

Closed
travisperson opened this issue Aug 10, 2016 · 11 comments
Closed

Selecting React Components via CSS prop selector #534

travisperson opened this issue Aug 10, 2016 · 11 comments

Comments

@travisperson
Copy link

I was looking at this yesterday and it appears that when using mount you can't use CSS prop selectors. I did some digging and found the PR (#70), and after reading through it a bit it appears that it was inertial. Instead of using a string (via CSS prop selector), the go to method was to instead use the object selector. At this point I stopped digging and assumed that the CSS prop selector was not supported in this way. However, when shallow rendering you can use a CSS prop selector with a React Component.

diff --git a/test/ReactWrapper-spec.jsx b/test/ReactWrapper-spec.jsx
index 503eee3..c8d7a65 100644
--- a/test/ReactWrapper-spec.jsx
+++ b/test/ReactWrapper-spec.jsx
@@ -384,6 +384,20 @@ describeWithDOM('mount', () => {

     });

+    it('should find a React Component based on a component props via css selector', () => {
+      class Foo extends React.Component {
+        render() { return <div />; }
+      }
+      const wrapper = mount(
+        <div>
+          <Foo bar="baz" />
+        </div>
+      );
+
+      expect(wrapper.find('Foo[bar="baz"]')).to.have.length(1);
+      expect(wrapper.find('[bar]')).to.have.length(1);
+    });
+
     it('should not find components with invalid attributes', () => {
       // Invalid attributes aren't valid JSX, so manual instantiation is necessary
       const wrapper = mount(
diff --git a/test/ShallowWrapper-spec.jsx b/test/ShallowWrapper-spec.jsx
index 47b570f..faf83b5 100644
--- a/test/ShallowWrapper-spec.jsx
+++ b/test/ShallowWrapper-spec.jsx
@@ -481,6 +481,21 @@ describe('shallow', () => {
       expect(wrapper.find('[data-foo]')).to.have.length(1);
     });

+    it('should find a React Component based on a component props via css selector', () => {
+      class Foo extends React.Component {
+        render() { return <div />; }
+      }
+
+      const wrapper = shallow(
+        <div>
+          <Foo bar="baz" />
+        </div>
+      );
+
+      expect(wrapper.find('Foo[bar="baz"]')).to.have.length(1);
+      expect(wrapper.find('[bar]')).to.have.length(1);
+    });
+
     it('should find components with multiple matching react props', () => {
       function noop() {}
       const wrapper = shallow(
  495 passing (2s)
  10 pending
  1 failing

  1) (uses jsdom) mount .find(selector) should find a React Component based on a component props via css selector:
     AssertionError: expected { Object (component, root, ...) } to have a length of 1 but got 0
      at Context.<anonymous> (ReactWrapper-spec.jsx:397:54)

Is this intentional for shallow but not for mount?

@blainekasten
Copy link
Contributor

selectors should always have parity between mount and shallow. So no, it would not be intentional. If I had to guess, it's the first selector that is failing? Foo[bar="baz"] and I would guess that it's due to the compound selector part. But we need to reproduce this. I'm pretty sure this used to work. I wonder if something regressed...

@aweary
Copy link
Collaborator

aweary commented Aug 10, 2016

@blainekasten @travisperson I've reproduced the issue here: enzyme-test-repo/blob/issue-534/test.js

class Foo extends React.Component {
  render() {
    return <div />
  }
}

describe('CSS selector parity', () => {
  it('should work with mount', () => {
    const wrapper = mount(
      <div>
          <Foo bar='baz' />
      </div>
    );
    expect(wrapper.find('Foo[bar="baz"]')).to.have.length(1);
  });
  it('should work with shallow', () => {
    const wrapper = shallow(
      <div>
          <Foo bar='baz' />
      </div>
    );
    expect(wrapper.find('Foo[bar="baz"]')).to.have.length(1);
  });
})

I tested with enzyme 2.2 - latest and it failed in all versions, so this doesn't appear to be a regression.

@aweary
Copy link
Collaborator

aweary commented Aug 11, 2016

@blainekasten so the issue is that instHasProperty returns early if the instance isn't a DOM component

export function instHasProperty(inst, propKey, stringifiedPropValue) {
  if (!isDOMComponent(inst)) return false;
...

These same checks aren't done in the corresponding ShallowTraversal methods, hence the incongruity. I'm sure there's a good reason that this check exists, but it does introduce this inconsistency. I think shallow's behavior is correct, so we'll have to discuss what the implications of removing this check might be.

@blainekasten
Copy link
Contributor

blainekasten commented Aug 11, 2016

Well, if it's any indication: I just commented that line out and ran the tests.

😕 T h e y A l l P a s s e d 😕

@blainekasten
Copy link
Contributor

Also, really great investigation work @aweary ! :hattip:

@travisperson
Copy link
Author

I'm sure there's a good reason that this check exists, but it does introduce this inconsistency.

The line as added during the PR for the feature. @blainekasten you might be able to pull some context out of the discussion, but I'd assume it may of just slipped by during the churn for on exactly how the selectors should work? I did not see any comments in the PR related to line.

https://github.com/airbnb/enzyme/pull/70/files#diff-077191fec7d029ff150ed4dfafc6a732R73

The original commit that the line was added in on (prior to the rebase) 2132d4e (first commit in the chain prior to rebase e5a035d if you want to back track and explore the history of the PR)

@aweary
Copy link
Collaborator

aweary commented Aug 11, 2016

I believe it was likely because the other selector methods instHasClassName and instHasId have that check, so it was meant to keep that consistent. But while you can't use class/id selectors on composite components, you can still query for props, so it can probably just be removed.

@blainekasten
Copy link
Contributor

Awesome. Sounds like a really easy fix. @travisperson would you be interested in submitting a PR for this? Just remove that one line and include some tests that would fail without the change.

travisperson pushed a commit to travisperson/enzyme that referenced this issue Aug 11, 2016
Selector such as `Foo[bar="baz"]` previously would not return any
components when rendered with Mount.

Resolves enzymejs#534
travisperson pushed a commit to travisperson/enzyme that referenced this issue Aug 11, 2016
Selector such as `Foo[bar="baz"]` previously would not return any
components when rendering with `mount`.

Resolves enzymejs#534
@kutyel
Copy link

kutyel commented Oct 17, 2016

Is this finished? What is left to do? Can I give it a go? Happy #hacktoberfest!

@travisperson
Copy link
Author

Sort of, there is a PR #538, but I'm not sure if the exact before has been settled on. There is a bit more information in the PR.

@ljharb
Copy link
Member

ljharb commented Sep 26, 2017

Closed by enzyme v3

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

No branches or pull requests

5 participants