-
Notifications
You must be signed in to change notification settings - Fork 257
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
feat(find): allow chaining find with findComponent #897
feat(find): allow chaining find with findComponent #897
Conversation
@@ -15,6 +18,16 @@ export default class BaseWrapper<ElementType extends Element> { | |||
this.wrapperElement = element | |||
} | |||
|
|||
abstract find(selector: string): DOMWrapper<Element> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how we set our expectations on both kind of wrappers (DOMWrapper & VueWrapper) by leveraging abstract methods
} | ||
|
||
findAllComponents(selector: FindAllComponentsSelector): VueWrapper<T>[] { | ||
findAllComponents(selector: FindAllComponentsSelector): VueWrapper<any>[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced VueWrapper<T>
with VueWrapper<any>
. While this typing could be definitely improved (I hate any
anywhere 🤣 ) it is at least "not incorrect" - <T>
here was a type of current instance, so if you've done
const wrapper = mount(Hello);
const cmps = wrapper.findAll(Foo);
cmps was wrongly typed to VueWrapper<Hello>[]
instead of VueWrapper<Foo>[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch! But any
feels definitely wrong.
Would you mind opening a dedicated PR to fix this properly (or we can file an issue)?
I think it would be better to at least let developers provide the type like wrapper.findAllComponents<Foo>(Foo)
to get Array<VueWrapper<Foo>>
. Or even better infer it from the selector if possible (but I'm not sure it's doable in all cases, as the developer may specify { name: 'Foo' }
as a selector).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree. I've opened separate issue #899 because I can't estimate effort to fix this properly, yet current PR has value - it is LESS wrong than previous version.
Hm, I am not sure about adding a feature because someone "urgently" needs it. If we have learned anything from the past, it's that if a feature is merged, documented or not, people will use it. Is this really a good idea? Also will need a rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments around TS soundness.
As Lachlan, I'm not sure about changing the API. Even if I understand the underlying need, I'm afraid it adds unneeded complexity to support tests cases that could be written otherwise. But I can totally accept merging it if the majority thinks it is a good addition 😉
} | ||
|
||
findAllComponents(selector: FindAllComponentsSelector): VueWrapper<T>[] { | ||
findAllComponents(selector: FindAllComponentsSelector): VueWrapper<any>[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch! But any
feels definitely wrong.
Would you mind opening a dedicated PR to fix this properly (or we can file an issue)?
I think it would be better to at least let developers provide the type like wrapper.findAllComponents<Foo>(Foo)
to get Array<VueWrapper<Foo>>
. Or even better infer it from the selector if possible (but I'm not sure it's doable in all cases, as the developer may specify { name: 'Foo' }
as a selector).
What do you think?
805ac7d
to
809b58b
Compare
Let me explain the rationale behind this change. First of all, quote @lmiller1990 from vuejs/vue-test-utils#1703 (comment):
So original deprecation of chaining were issues in this repository with chaining implementation This change adds a lot of value for Some details where `shallowMount` plays extremely well for us`shallowMount` approach now really shines with our effort of migrating from Vue2 to Vue3 compat build. At GitLab scale things could go wrong for tons of different reasons (we've already discovered multiple not-documented incompatibilities between Vue2 and Vue3-compat, so `mount` tests are ... just 🔴 red with usually useless error somewhere deep inside Vue.
I believe everyone is on the same page that one of the key ideas of VTU is to help people in writing tests in "semantical way" providing simple mental model of how VTU works (without knowing all dark magic underneath 🤣 ). Basing on this assumption:
const targetCell = wrapper.find('tr:nth-child(2) .actions');
const actionBar = wrapper.findAllComponents(ActionBar).find(w => targetCell.includes(w.element)) For me this is semantically bad, compared to: const actionBar = wrapper.find('tr:nth-child(2) .actions').findComponent(ActionBar) |
Thanks for the detailed explanation @xanf ❤️ |
A little more historical context: I think chaining these together is not very ideal, but compromises need to happen, especially in a core lib that is supposed to be suitable for everyone's purposes. As a compromise, we can have this feature but choose not to highlight in the documentation heavily. It's more of a "here it is, if you know it exists, use it at your own digression". |
This is a follow-up (lol) of vuejs/vue-test-utils#1703 (comment)
Chaining
.find
with.findComponent
has multiple usages in GitLab and other projects (for examplebootstrap-vue
) (see vuejs/vue-test-utils#1703 (comment)) and actually was a blocker for us to move our API fromfind
tofindComponent
in VTU v1 :)This PR adds chaining to VTU v2. Additionally it enables "workaround" if someone urgently need to find some known component by CSS selector as mentioned in #689 (assuming #896 is merged):
will match
<Hello class="foo" />
. I like both that we have possibility to do that if really needed (for example for gradual migration) and that it looks & feels ugly so people with high probability will stop and re-think their approach