-
Notifications
You must be signed in to change notification settings - Fork 245
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
Request to reinstate ability to use CSS selectors in findComponent #904
Comments
With #897 being merged you can do this one:
I would like to oppose this proposal - I really like simplicity of mental model - selecting by query selectors always return DOM nodes and I consider workaround mentioned on top sufficient for any "extreme" use cases. The main downside of selecting component by CSS selector is that actually multiple components can match single selector (see #689 for more context) |
Ah I didn't know that chaining |
@dospunk totally understand you. We also used a lot of chaining DOM selectors to find componnents in pre-1.0 versions of vtu and now it's time to pay |
I am using this alot, so I can use the attribute and selector for unit and E2E tests. E.g: <template>
<TextButton label="Delete" data-test="delete-button"/>
<TextButton label="Edit" data-test="edit-button"/>
</template> const deleteButton = wrapper.findComponent('[data-test=delete-button')
const editButton = wrapper.findComponent('[data-test=edit-button') Now I need to use the DOM selector for const deleteButton = wrapper.find('[data-test=delete-button').findComponent({ name: 'TextButton' })
const editButton = wrapper.find('[data-test=edit-button').findComponent({ name: 'TextButton' }) Unfortunately this is not possible yet, because #897 is not in |
Oops, sorry for misleading information about #897 - I'm using own fork which is far ahead of master, so totally forgot to check if this was released |
Slightly related: chaining As for the feature request, I don't like the idea of mixing I am interested in why @xanf opposes this, but does not oppose |
In my opinion the biggest pros of finding components using selectors was the ability to strictly separate test and dev contexts. Using Using component names or refs we make unnecessery dependency in our tests and make it more bugprone in case of any refactors. I understand the argument of not mixing DOM selectors with VDOM selectors but still consider it as weaker against the idea of mixing dev and test context using same names / refs. What's more as @lmiller1990 noticed using "trick" with So I also postulate to reinstate ability to use CSS selectors in getComponent/findComponent :) |
Inclined to agree - opinions on mixing DOM/VDOM aside, if we have I'd still like to get some opposite opinions, mainly @xanf, but seems like this is something we'd want to do pre 2.0.0 (I don't see much else blocking a full 2.0.0, it'd be great to get out of the rc stage). |
I also would like to recive some opposite arguments :) So if anyone have any please share them. Just for now I downgrade our project to ver rc.12 to use query selectors but still hope that in pre 2.0.0 it will be reinstated :) |
I have to agree with @freakzlike , the ability to be able to target a component using a A component may change, for example a "RaisedButton" may change for a "FlatButton", which would break the test. Instead I can target the underlying component with This is also very valuable like he said, when building everything to also work in the same way with e2es. RC 13 has a lot of very valuable fixes and improvements that have a lot of my tests locked with a |
I agree with what @marina-mosti said. For now, I am changing my tests as such:
It's a bit of a pain, but it works in my case. That said, this is just for an app developed as part of a training we give our customers, not a real app. I could just as easily instruct the customers to stay on Personally, I would just add a
|
Let's just support CSS with Would someone like to make a PR? |
@lmiller1990 I'll handle that for both versions of VTU (actually it's kind of confusing that this discussion was never raised in VTU v1 repo) |
Thanks @lmiller1990 @xanf much appreciated. Perhaps there should be a section for best practices in the docs that explains the problems with using CSS selector? LMK if you want some help with that 👍🏻 |
Hard to say if there is really "problems" with using CSS selectors to find components - if it works, there's not real problem. To me, it feels more like philosophical differences. Always good to improve the docs, though, so if you think they can be better, please make them better 🚀 @xanf if you want to pick this up, that would be great! |
Short status update for everyone involved: ETA for pull requests: tomorrow :) |
After this and a few |
@lmiller1990
Am I correct that we're expecting to silently ignore divs and return array with 1 element for |
@marina-mosti @lmiller1990 @afontcu @cexbrayat I would like to have your input here. Let the code speak for me: it('findComponent returns top-level component when multiple components are matching', () => {
const DeepNestedChild = {
name: 'DeepNestedChild',
template: '<div>I am deeply nested</div>'
}
const NestedChild = {
name: 'NestedChild',
components: { DeepNestedChild },
template: '<deep-nested-child class="in-child" />'
}
const RootComponent = {
name: 'RootComponent',
components: { NestedChild },
template: '<div><nested-child class="in-root"></nested-child></div>'
}
const wrapper = mountingMethod(RootComponent, { stubs: { NestedChild } })
expect(wrapper.findComponent('.in-root').vm.$options.name).toEqual('NestedChild')
// someone might expect DeepNestedChild here, but
// we always return TOP component matching DOM element
expect(wrapper.findComponent('.in-child').vm.$options.name).toEqual('NestedChild')
}) (this code is taken from my tests for VTU v1, so One might obviously expect that Also in the light of #689 which was original issue we have this case: it('findAllComponents returns top-level components when components are nested', () => {
const DeepNestedChild = {
name: 'DeepNestedChild',
template: '<div>I am deeply nested</div>'
}
const NestedChild = {
name: 'NestedChild',
components: { DeepNestedChild },
template: '<deep-nested-child class="in-child" />'
}
const RootComponent = {
name: 'RootComponent',
components: { NestedChild },
template: '<div><nested-child class="in-root"></nested-child></div>'
}
const wrapper = mountingMethod(RootComponent, { stubs: { NestedChild } })
expect(wrapper.findAllComponents('.in-root')).toHaveLength(1)
expect(
wrapper.findAllComponents('.in-root').at(0).vm.$options.name
).toEqual('NestedChild')
expect(wrapper.findAllComponents('.in-child')).toHaveLength(1)
// someone might expect DeepNestedChild here, but
// we always return TOP component matching DOM element
expect(
wrapper.findAllComponents('.in-child').at(0).vm.$options.name
).toEqual('NestedChild')
}) Theoretically both Something like (including an example above):
WDYT? |
After certain research & discussions - I feel it's ok to proceed just with this warning in docs - this should be quite rare edge case:
So in terms of general usability it should not be affecting us to much, still worth mentioning in comments, though |
Hey @xanf I agree. I am not a fan of Class in particular is a complicated way to target stuff even for units, and I'm sure there's people doing it but with the way that attrs.class is working now I would definitely advise or warn about it as you said. Thanks for pushing this issue to the finish line 🙌🏻 |
@marina-mosti the drama is far more bigger than it feels :) it('findComponent returns top-level components when components are nested', () => {
const DeepNestedChild = {
name: 'DeepNestedChild',
template: '<div>I am deeply nested</div>'
}
const NestedChild = {
name: 'NestedChild',
components: { DeepNestedChild },
template: '<deep-nested-child data-testid="child" />'
}
const RootComponent = {
name: 'RootComponent',
components: { NestedChild },
template: '<nested-child/>'
}
const wrapper = mount(RootComponent);
expect(
wrapper.findComponent('[data-testid=child]').vm.$options.name
).toEqual('RootComponent') // yes, Root, not DeepNestedChild :bomb: |
@xanf yikes. Honestly, I never ran into these type of issues before at work because we very very very rarely use The unit test for |
@marina-mosti same here, I'm |
Yes, I think this makes sense... I think that all the APIs should work with both Regarding the edge cases, I think we have to accept these will exist and just document them. Most people won't run into them, and if they do, hopefully the docs can provide some useful guidance. |
It said in the release notes that if we felt strongly about this feature we could open an issue, so that's what I'm doing. I don't think it makes sense to remove the ability to use CSS selectors in findComponent just to make VTU2 more in line with VTU1, and I think there are some notable benefits to using CSS selectors to find components.
The most significant benefit (in my opinion) is the ability to more specifically select components. Say I have a component like so, that imports a Spinner component and then shows it on two buttons depending on which is clicked
Without CSS selectors I have to use
findAllComponents(Spinner)[1]
to access Spinner B, which will break if the buttons switch positions, if there are any other spinners in any other components before this component on the page, etc. With large components or views this can become a nightmare of remembering state for completely unrelated parts of the page.It was also just nice to have another option for selecting components that didn't come with the downsides of the other three. Selecting by constructor requires you to import the constructor into your test, selecting by name requires you to know the name of the component which isn't always readily available or even present for 3rd party components, and selecting by ref requires you to add refs to components that don't already have them just for testing. Personally I like to use data attributes for selectors in tests since they can be easily removed in production.
The text was updated successfully, but these errors were encountered: