Allow find to work on both Pascal case and camel case #1398
Conversation
lmiller1990
changed the base branch from
dev
to
feat/allow-testing-src-directly
lmiller1990
changed the base branch from
feat/allow-testing-src-directly
to
dev
packages/test-utils/src/matches.js
Outdated
|
|
||
| export function vmMatchesName(vm, name) { | ||
| function vmMatchesName(vm, name) { | ||
| console.log(name) |
There was a problem hiding this comment.
some debugging stuff over here! ;)
There was a problem hiding this comment.
Do we want a lint rule for no console log? We do this at work. Same for debugger statements
There was a problem hiding this comment.
I think its OK to have one yeah :) With the new linting on commit, it will warn you.
packages/test-utils/src/matches.js
Outdated
| vm.$options && vm.$options.name === capitalize(name) || | ||
| vm.$options && vm.$options.name && vm.$options.name === capitalize(camelize(name)) || | ||
| vm.$options && vm.$options.name && capitalize(camelize(vm.$options.name)) === name | ||
| ) |
There was a problem hiding this comment.
I have a proposal, lets save the vm.$options.name to a variable, something like
const componentName = vm.$options && vm.$options.name || ''then we can easily reduce those those lines. Yes it will use a bit more memory, but for readability sake I think it makes sense?
Also that line 18 has an extra check others dont.
Unrelated:
You know whats the diff between vm.name and vm.$options.name? One is the one you set in its options and the other is the one that it is registered by?
There was a problem hiding this comment.
Can we add comments for each of the conditionals? Eg what use case each line is handling? A 7 line if statement is hard to grok easily.
There was a problem hiding this comment.
Yep agreed, I'll make these changes
There was a problem hiding this comment.
@dobromir-hristov I am not sure on the difference between vm.name and vm.$options.name. vm.name is undefined in every test... I removed it and everything is still passing. If it serves no purpose, we may as well omit that check. What do you think?
We should check the source code for Vue and see if we can figure out the difference.
Regarding your comment about the extra variable - I agree, that is more readable. I updated it the code.
@JessicaSachs I added some comments explaining why we do each of the matches. What do you think? If you think you can make the comment clearer, you can add some commits or I can add some if you leave an updated comment here.
There was a problem hiding this comment.
I also gave a quick search and did not see vm.name defined. Its not in the Vue types as well.
Wonder if its something from "old days"?
New code is much more readable, gj
| export function vmMatchesName(vm, name) { | ||
| function vmMatchesName(vm, name) { | ||
| // We want to mirror how Vue resolves component names in SFCs: | ||
| // For example, <test-component />, <TestComponent /> and `<testComponent /> |
There was a problem hiding this comment.
| // For example, <test-component />, <TestComponent /> and `<testComponent /> | |
| // For example, <test-component />, <TestComponent /> and <testComponent /> |
| expect(wrapper.find({ name: 'camel-case' }).name()).to.equal('CamelCase') | ||
| }) | ||
|
|
||
| it('returns a Wrapper matching a Pascal Case name option and a kebab-casecomponent name ', () => { |
There was a problem hiding this comment.
| it('returns a Wrapper matching a Pascal Case name option and a kebab-casecomponent name ', () => { | |
| it('returns a Wrapper matching a PascalCase name option and a kebab-case component name', () => { |
| expect(wrapper.find({ name: 'camelCase' }).name()).to.equal('CamelCase') | ||
| }) | ||
|
|
||
| it('returns a Wrapper matching a kebab-case name option and a Pascal Case component name ', () => { |
There was a problem hiding this comment.
| it('returns a Wrapper matching a kebab-case name option and a Pascal Case component name ', () => { | |
| it('returns a Wrapper matching a kebab-case name option and a PascalCase component name', () => { |
| @@ -431,6 +431,33 @@ describeWithShallowAndMount('find', mountingMethod => { | |||
| ) | |||
| }) | |||
|
|
|||
| it('returns a Wrapper matching a camelCase name option and a Pascal Case component name ', () => { | |||
There was a problem hiding this comment.
| it('returns a Wrapper matching a camelCase name option and a Pascal Case component name ', () => { | |
| it('returns a Wrapper matching a camelCase name option and a PascalCase component name', () => { |
resolves #1232
I think it's fine to support finding a component named
TestComponentwithtest-componentand vice versa, or eventtestComponent- especially for third party packages, it can be hard to know thenameof a component. Just makesfinda bit more forgiving and improves the DX with no real downside.I forked from #1396, will retarget once that is merged. I love the new workflow (run tests against
src).