Skip to content

Find / FindRef: wrapper function to find element based on ref #67

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

Closed
matt-oconnell opened this issue Oct 2, 2017 · 15 comments
Closed

Comments

@matt-oconnell
Copy link
Contributor

matt-oconnell commented Oct 2, 2017

Suggestion for additional functionality

Currently, there is no functionality built into vue-test-utils to select elements within a component based on $refs and return them within a Wrapper. Our test suite is built around the idea that we can easily tag elements we need to hook onto within tests using $ref rather than some arbitrary "marker class."

I think it might be helpful to include a specific method on the wrapper like findRefs

I'm not sure if this is a widely used strategy but we've found it helpful. Template structures change often and class selectors can easily break or target the wrong element when these changes happen.

@eddyerburgh
Copy link
Member

eddyerburgh commented Oct 2, 2017

I think this is a good idea, but I don't think we should add an extra method.

We could extend find and findAll, so that they could take a selector object.

find({
  selector: 'refs',
  value: 'value' 
})

Then we could extend it to other selectors, like props.

find({
  selector: 'props',
  value: 'value' 
})

@matt-oconnell
Copy link
Contributor Author

I wonder how many alternate use cases we would have for a selector other than CSS, component name, or $ref. I'm not sure if component with prop: xwould ever be used as a selector, but I could be wrong.

@matt-oconnell
Copy link
Contributor Author

But, if we do want to go with the selector/value find/findAll functions, I could start a PR. Initially it could look for $props / $refs. I guess the tough part would be matching $props because the values could be any

@eddyerburgh
Copy link
Member

I think we should just add support for refs at the moment ☺

@matt-oconnell
Copy link
Contributor Author

@eddyerburgh To make it a little more concise, how do you feel about:

find({ ref: 'myRefName' })

@eddyerburgh
Copy link
Member

Yep that looks good 👍

@matt-oconnell
Copy link
Contributor Author

Great, updating the PR to reflect this. Currently, the validation error messages for find/findAll display: wrapper.find/findAll() must be passed a valid CSS selector or a Vue constructor. As we update the API to accept this new options object, how should we refer to it?

If we're going to start with refs for now, we could use:

wrapper.find() must be passed a valid CSS selector, Vue constructor, or ref object

Or wrapper.find() must be passed a valid CSS selector, Vue constructor, or options object

My only concern with the latter is that it may be too vague.

@eddyerburgh
Copy link
Member

Ah that's a good point.

If we add this then we need to:

  • Update all methods that take a selector: is, contains, find, findAll
  • Update the selectors page to include the refs object
  • Link to the selectors page in the validation error

That's a fairly big change. Before we go further, I would like some input from others.

Do we want to extend the selector to include { refs: 'refName' }

@matt-oconnell
Copy link
Contributor Author

So, one thing other thing to note: When selecting $refs, we rely on the $ref property to be available on the wrapper.vm, because we want to limit the results to this scope. This is one of the benefits of $refs. If we are using a wrapper on a non-Vue HTML Element, we should not allow selecting by $ref.

@eddyerburgh
Copy link
Member

eddyerburgh commented Oct 4, 2017 via email

@skray
Copy link
Contributor

skray commented Nov 2, 2017

The PR for this change mentioned wanting some feedback from the community, so I thought I'd chime in.

This would be great to have, I liked the initial suggestion of having wrapper.ref, but I understand from a consistency standpoint why you'd want to extend the current finders. The current recommendation of using wrapper.find({refs: 'refName'}) feels comfortable to me.

@eddyerburgh
Copy link
Member

eddyerburgh commented Nov 2, 2017

Thanks for the feedback @skray.

I'm happy for this work to continue. @matt-oconnell do you have any blockers?

@matt-oconnell
Copy link
Contributor Author

This is pretty much fully implemented in the corresponding PR. The blocker was that the Flow types were invalid (specifically Component,) so this PR fails the type check unless any is used

@eddyerburgh
Copy link
Member

I think we'll have to use any until proper flowtypes are exported from Vue — vuejs/vue#5027

@eddyerburgh
Copy link
Member

This can be closed, released in 1.0.0-beta.5

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

3 participants