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

Make .exists take optional selector #1695

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

krawaller
Copy link

This PR makes .exists optionally take a selector, in which case it will delegate to .find(selector).exists().

@krawaller krawaller changed the title Feature/exists selector Make .exists take optional selector Jun 29, 2018
@krawaller krawaller force-pushed the feature/exists_selector branch from 5207bb7 to 1d801c1 Compare June 29, 2018 16:56
@ljharb
Copy link
Member

ljharb commented Jun 29, 2018

Thanks for the PR - I'm not sure if this complexity is worth adding to the API, since .exists(x) is simple sugar for .find(x).exists(). Can you elaborate on how this is helpful?

@krawaller
Copy link
Author

I'm not sure either, and I very much appreciate the lean API and the effort that went into keeping it lean, so I'm rather happy you're hesitating.

Here are some arguments, ordered from crappy to somewhat good, why I'd still like to see this change included in Enzyme;

  • Throwing .exists onto a .find is a very common use case for it, so making that use case less clunky has some extra value.

  • The added complexity is rebated by the fact that we're just referring existing concepts - "selectors" and .find - so the cognitive cost of the change is rather small.

  • It is easy to intuitively expect .exists to take an argument. It just reads so well, and that kind of .has functionality is very common in libraries in similar problem space, but Enzyme lacks this.

  • Making that misunderstanding is dangerous. In a recent project (with crappy tooling lacking static type checking) I added a test in the line of wrapper.exists(selector).should.be.true(). The test passed, but without protecting against what it was supposed to, and that ended up blowing up in my client's face. My bad, but to some extent I did feel let down by the API.

And that last sob story is of course what prompted this PR. Now the question is just whether the story is sad enough! :)

@ljharb
Copy link
Member

ljharb commented Jun 30, 2018

If exists threw an exception when it received an argument, would you still want this API?

@krawaller
Copy link
Author

Oh, very good question! Hmm. Not sure.

Preventing the misunderstanding was definitely my main intent, so if an error had saved me from blowing my client's face off I probably wouldn't have made this PR.

The crappy parts of the arguments still stand however, and I think my immediate reaction to an error saying "Exists doesn't take an argument" would be "well, why the hell not".

As a very unscientific and solitaire data point; in our current fledgling project we have 18 uses of .exists, and every single one of them is preceeded by a .find. This also matches my previous experiences. The current lack of a .has/.contains method is a bit of an eyebrow-raiser.

My answer to your question: Yes, I'd still want this proposed API, but not as badly, and would be happy to rework the PR to make .exists throw an error, if you feel the crappy arguments are too crappy. But shortening 100% of the tests using .exists does still have quite the appeal to me.

@ljharb ljharb force-pushed the feature/exists_selector branch from 1d801c1 to 7fcd96e Compare June 30, 2018 20:55
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.

Thanks for bearing with me; I think I'm convinced. I made a few small tweaks, and rebased, and will merge shortly.

@ljharb ljharb merged commit 7fcd96e into enzymejs:master Jun 30, 2018
@krawaller
Copy link
Author

Ah, good catch adding the default arg!

btimo pushed a commit to btimo/enzyme that referenced this pull request Dec 10, 2018
.exists, since [enzymejs#1695](enzymejs#1695), accepts a optional selector. Unfortunately, this optional parameter was limited to the `string` type whereas the documentation inform us it can be any valid [EnzymeSelector](https://airbnb.io/enzyme/docs/api/selector.html).
This commit fixes this limitation by accepting any type for the parameter and using `.find()` internal functions to throw any necessary `TypeError`.
btimo pushed a commit to btimo/enzyme that referenced this pull request Dec 10, 2018
`.exists`, since [enzymejs#1695](enzymejs#1695), accepts an optional selector. Unfortunately, this optional parameter was limited to the `string` type whereas the documentation inform us it can be any valid [EnzymeSelector](https://airbnb.io/enzyme/docs/api/selector.html).
This commit fixes this limitation by accepting any type for the parameter and using `.find()` internal functions to throw any necessary `TypeError`.
btimo pushed a commit to btimo/enzyme that referenced this pull request Dec 10, 2018
`.exists`, since [enzymejs#1695](enzymejs#1695), accepts an optional selector. Unfortunately, this optional parameter was limited to the `string` type whereas the documentation inform us it can be any valid [EnzymeSelector](https://airbnb.io/enzyme/docs/api/selector.html).
This commit fixes this limitation by accepting any type for the parameter and using `.find()` internal functions to throw any necessary `TypeError`.
btimo pushed a commit to btimo/enzyme that referenced this pull request Dec 10, 2018
`.exists`, since [enzymejs#1695](enzymejs#1695), accepts an optional selector. Unfortunately, this optional parameter was limited to the `string` type whereas the documentation inform us it can be any valid [EnzymeSelector](https://airbnb.io/enzyme/docs/api/selector.html).
This commit fixes this limitation by accepting any type for the parameter and using `.find()` internal functions to throw any necessary `TypeError`.
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.

2 participants