Skip to content

Allow find to work on both Pascal case and camel case #1398

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 13, 2020
Merged

Allow find to work on both Pascal case and camel case #1398

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bff46f1
fix: allow find to work with any casing
lmiller1990 Jan 9, 2020
83ee6c1
fix: do not export function
lmiller1990 Jan 9, 2020
82bd743
fix: do not remove hyphen in component names
lmiller1990 Jan 10, 2020
8607c28
fix: only match by capitalize
lmiller1990 Jan 10, 2020
1718005
fix: cover both kebab and camel cases
lmiller1990 Jan 10, 2020
b3e2b92
fix: simplify code
lmiller1990 Jan 12, 2020
0472aa4
fix: add comments explaining vmMatchesName functino
lmiller1990 Jan 12, 2020
e8f3266
fix: update comment
lmiller1990 Jan 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions packages/test-utils/src/matches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,20 @@ import {
FUNCTIONAL_OPTIONS
} from 'shared/consts'
import { isConstructor } from 'shared/validators'
import { capitalize, camelize } from 'shared/util'

export function vmMatchesName(vm, name) {
function vmMatchesName(vm, name) {
console.log(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some debugging stuff over here! ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a lint rule for no console log? We do this at work. Same for debugger statements

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its OK to have one yeah :) With the new linting on commit, it will warn you.

console.log(vm.$options.name)
return (
!!name && (vm.name === name || (vm.$options && vm.$options.name === name))
!!name && (
vm.name === name ||
(vm.$options && vm.$options.name === name) ||
vm.name === capitalize(name) ||
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
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep agreed, I'll make these changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
}

Expand Down
29 changes: 28 additions & 1 deletion test/specs/wrapper/find.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,33 @@ describeWithShallowAndMount('find', mountingMethod => {
)
})

it('returns a Wrapper matching a camelCase name option and a Pascal Case component name ', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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', () => {

const component = {
name: 'CamelCase',
render: h => h('div')
}
const wrapper = mountingMethod(component)
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 ', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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', () => {

const component = {
name: 'CamelCase',
render: h => h('div')
}
const wrapper = mountingMethod(component)
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 ', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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', () => {

const component = {
name: 'camel-case',
render: h => h('div')
}
const wrapper = mountingMethod(component)
expect(wrapper.find({ name: 'CamelCase' }).name()).to.equal('camel-case')
})

it('returns Wrapper of Vue Component matching the ref in options object', () => {
const wrapper = mountingMethod(ComponentWithChild)
expect(wrapper.find({ ref: 'child' }).isVueInstance()).to.equal(true)
Expand All @@ -441,7 +468,7 @@ describeWithShallowAndMount('find', mountingMethod => {
const wrapper = mountingMethod(compiled)
const a = wrapper.find('a')
const message =
'[vue-test-utils]: $ref selectors can only be used on Vue component wrappers'
'[vue-test-utils]: $ref selectors can used on Vue component wrappers'
const fn = () => a.find({ ref: 'foo' })
expect(fn)
.to.throw()
Expand Down