Skip to content
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

Jump to bottom

feat: split attrs and props mounting options #99

Merged
merged 3 commits into from
May 2, 2020
Merged

feat: split attrs and props mounting options #99

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f5ae12
feat: split attrs and props mounting options
cexbrayat Apr 28, 2020
d271a12
test: split moutingProps.attrs tests
cexbrayat May 1, 2020
ff1a72c
test: extra mouting props can be given to props if cast
cexbrayat May 2, 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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ component | ✅ | (new!) nested in [`global`](https://vuejs.github.io/vue-test-u
directives | ✅ | (new!) nested in [`global`](https://vuejs.github.io/vue-test-utils-next-docs/api/#global)
stubs | ✅
attachToDocument |✅| renamed `attachTo`. See [here](https://github.com/vuejs/vue-test-utils/pull/1492)
attrs | ⚰️ | use `props` instead, it assigns both attributes and props.
attrs | ✅
scopedSlots | ⚰️ | scopedSlots are merged with slots in Vue 3
context | ⚰️ | different from Vue 2, does not make sense anymore.
localVue | ⚰️ | may not make sense anymore since we do not mutate the global Vue instance in Vue 3.
Expand Down
33 changes: 14 additions & 19 deletions src/mount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type Slot = VNode | string | { render: Function }
interface MountingOptions<Props> {
data?: () => Record<string, unknown>
props?: Props
attrs?: Record<string, unknown>
slots?: {
default?: Slot
[key: string]: Slot
Expand All @@ -47,37 +48,27 @@ type ExtractComponent<T> = T extends { new (): infer PublicInstance }
: any

// Component declared with defineComponent
export function mount<
TestedComponent extends ComponentPublicInstance,
PublicProps extends TestedComponent['$props']
>(
export function mount<TestedComponent extends ComponentPublicInstance>(
originalComponent: { new (): TestedComponent } & Component,
options?: MountingOptions<PublicProps>
options?: MountingOptions<TestedComponent['$props']>
): VueWrapper<TestedComponent>
// Component declared with { props: { ... } }
export function mount<
TestedComponent extends ComponentOptionsWithObjectProps,
PublicProps extends ExtractPropTypes<TestedComponent['props']>
>(
export function mount<TestedComponent extends ComponentOptionsWithObjectProps>(
originalComponent: TestedComponent,
options?: MountingOptions<PublicProps>
options?: MountingOptions<ExtractPropTypes<TestedComponent['props'], false>>
): VueWrapper<ExtractComponent<TestedComponent>>
// Component declared with { props: [] }
export function mount<
TestedComponent extends ComponentOptionsWithArrayProps,
PublicProps extends Record<string, any>
>(
export function mount<TestedComponent extends ComponentOptionsWithArrayProps>(
originalComponent: TestedComponent,
options?: MountingOptions<PublicProps>
options?: MountingOptions<Record<string, any>>
): VueWrapper<ExtractComponent<TestedComponent>>
// Component declared with no props
export function mount<
TestedComponent extends ComponentOptionsWithoutProps,
ComponentT extends ComponentOptionsWithoutProps & {},
PublicProps extends Record<string, any>
ComponentT extends ComponentOptionsWithoutProps & {}
>(
originalComponent: ComponentT extends { new (): any } ? never : ComponentT,
options?: MountingOptions<PublicProps>
options?: MountingOptions<never>
): VueWrapper<ExtractComponent<TestedComponent>>
export function mount(
originalComponent: any,
Expand Down Expand Up @@ -121,7 +112,11 @@ export function mount(

// we define props as reactive so that way when we update them with `setProps`
// Vue's reactivity system will cause a rerender.
const props = reactive({ ...options?.props, ref: MOUNT_COMPONENT_REF })
const props = reactive({
...options?.attrs,
...options?.props,
ref: MOUNT_COMPONENT_REF
})

// create the wrapper component
const Parent = defineComponent({
Expand Down
5 changes: 0 additions & 5 deletions src/vue-shims.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,3 @@ declare module '*.vue' {
import Vue from 'vue'
export default any
}

declare module 'vue' {
Copy link
Member

Choose a reason for hiding this comment

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

We could use this:

declare module '*.vue' {
  import { ComponentOptions } from 'vue'
  var component: ComponentOptions
  export default component
}

import Vue from 'vue/dist/vue'
export = Vue
}
31 changes: 20 additions & 11 deletions test-dts/mount.d-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,25 @@ const AppWithDefine = defineComponent({
a: {
type: String,
required: true
}
},
b: Number
},
template: ''
})

// accept props
let wrapper = mount(AppWithDefine, {
props: { a: 'Hello' }
props: { a: 'Hello', b: 2 }
})
// vm is properly typed
expectType<string>(wrapper.vm.a)

// can receive extra props
// ideally, it should not
// but the props have type { a: string } & VNodeProps
// which allows any property
mount(AppWithDefine, {
props: { a: 'Hello', b: 2 }
props: { a: 'Hello', c: 2 }
})

// wrong prop type should not compile
Expand All @@ -48,10 +52,12 @@ wrapper = mount(AppWithProps, {
// vm is properly typed
expectType<string>(wrapper.vm.a)

// can receive extra props
mount(AppWithProps, {
props: { a: 'Hello', b: 2 }
})
// can't receive extra props
expectError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this error out for transparent components without props defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you give me an example of what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

If a component has no props defined, and passes them down via $attrs.

const Component = {
   template: `<div><ChildComponent v-bind="$attrs"/></div>`
}
const wrapper = mount(Component, { props: { a:'a' } })

expect(wrapper.findComponent(ChildComponent).props('a')).toBe('a')

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so yes: I expect this to error out if the props are not defined (as they end up with the never type). I think it makes sense to error out, as it is not the most common case, and let the developer use mount(Component, { props: { a:'a' } as any }) in that case. But I'm open to your suggestions if you think this is too constraint.

Copy link
Contributor

Choose a reason for hiding this comment

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

This wont error out for plain JS use right?

Erroring out is technically wrong, as the Vue API allows you to pass props to that kind of components, but as long as we state the reasoning behind this, should be fine :)

You are right, its not a common use case and having type safety for the 80% of people is better, we just need to make sure these little cases have workarounds for the more novice TS users out there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is for TS developers only, JS will be fine, sorry if that wasn't clear :)

And even in TS it still works, it just forces the developers to cast the props if they want to use it with undeclared props.

IMO, TS developers will be happy to have intellisense/autocomplete/type-checking in tests, and won't be too lost if the compilation indicates that the props does not match the props of the component. But you're 100% right that we need to mention it the docs 👍

mount(AppWithProps, {
props: { a: 'Hello', b: 2 }
})
)

// wrong prop type should not compile
expectError(
Expand All @@ -73,6 +79,7 @@ wrapper = mount(AppWithArrayProps, {
expectType<string>(wrapper.vm.a)

// can receive extra props
// as they are declared as `string[]`
mount(AppWithArrayProps, {
props: { a: 'Hello', b: 2 }
})
Expand All @@ -81,7 +88,9 @@ const AppWithoutProps = {
template: ''
}

// can receive extra props
wrapper = mount(AppWithoutProps, {
props: { b: 'Hello' }
})
// can't receive extra props
expectError(
(wrapper = mount(AppWithoutProps, {
props: { b: 'Hello' }
}))
)
57 changes: 57 additions & 0 deletions tests/mountingOptions/attrs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { defineComponent, h } from 'vue'
import { mount } from '../../src'

describe('mountingOptions.attrs', () => {
const Component = defineComponent({
props: {
message: {
type: String,
required: true
},
otherMessage: {
type: String
}
},

render() {
return h('div', {}, `Message is ${this.message}`)
}
})

test('assigns extra attributes on components', () => {
const wrapper = mount(Component, {
props: {
message: 'Hello World'
},
attrs: {
class: 'HelloFromTheOtherSide',
id: 'hello',
disabled: true
}
})

expect(wrapper.attributes()).toEqual({
class: 'HelloFromTheOtherSide',
disabled: 'true',
id: 'hello'
})

expect(wrapper.props()).toEqual({
message: 'Hello World'
})
})

test('assigns event listeners', async () => {
const Component = {
template: '<button @click="$emit(\'customEvent\', true)">Click</button>'
}
const onCustomEvent = jest.fn()
const wrapper = mount(Component, { attrs: { onCustomEvent } })
const button = wrapper.find('button')
await button.trigger('click')
await button.trigger('click')
await button.trigger('click')

expect(onCustomEvent).toHaveBeenCalledTimes(3)
})
})
58 changes: 24 additions & 34 deletions tests/mountingOptions/props.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
import { defineComponent, h } from 'vue'
import WithProps from '../components/WithProps.vue'
import { mount } from '../../src'

describe('mountingOptions.props', () => {
test('passes props', () => {
const Component = defineComponent({
props: {
message: {
type: String,
required: true
}
const Component = defineComponent({
props: {
message: {
type: String,
required: true
},

render() {
return h('div', {}, `Message is ${this.message}`)
otherMessage: {
type: String
}
})
},

render() {
return h('div', {}, `Message is ${this.message}`)
}
})
test('passes props', () => {
const wrapper = mount(Component, {
props: {
message: 'Hello'
Expand All @@ -25,38 +26,27 @@ describe('mountingOptions.props', () => {
expect(wrapper.text()).toBe('Message is Hello')
})

test('assigns extra attributes on components', () => {
const wrapper = mount(WithProps, {
test('assigns extra properties as attributes on components', () => {
// the recommended way is to use `attrs` though
// and ideally it should not even compile, but props is too loosely typed
// for components defined with `defineComponent`
const wrapper = mount(Component, {
props: {
message: 'Hello World',
class: 'HelloFromTheOtherSide',
id: 'hello',
disabled: true,
msg: 'Hello World'
disabled: true
}
})

expect(wrapper.props()).toEqual({
message: 'Hello World'
})

expect(wrapper.attributes()).toEqual({
class: 'HelloFromTheOtherSide',
disabled: 'true',
id: 'hello'
})

expect(wrapper.props()).toEqual({
msg: 'Hello World'
})
})

test('assigns event listeners', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically the right way to attach a listener, not through attrs, even though they are identical as of now.

Copy link
Member Author

@cexbrayat cexbrayat May 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So your point is that we should keep the possibility of assigning extra props for this use-case?
There are several options:

  • do not allow it as it is in this PR, forcing the user to cast the props as any in the test, if he/she really wants to use props. I can change the test with a cast to let it in the current file.
  • allow it, but we lose a it of type safety as we can't make a difference between a badly written prop and an extra prop
  • allow it via some other possibility (mountingProps.extraProps ?) as it is not the most common use-case
    What do you think would be the most sound strategy?

Copy link
Contributor

Choose a reason for hiding this comment

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

So extra props would be attrs basically? If so, then I say just mention in the docs that people can use attrsfor that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can nudge the developers in the direction we feel makes more sense in the docs, either:

  • use attrs for extra props
  • or use props but cast with as any to indicate this is intentional.

const Component = {
template: '<button @click="$emit(\'customEvent\', true)">Click</button>'
}
const onCustomEvent = jest.fn()
const wrapper = mount(Component, { props: { onCustomEvent } })
const button = wrapper.find('button')
await button.trigger('click')
await button.trigger('click')
await button.trigger('click')

expect(onCustomEvent).toHaveBeenCalledTimes(3)
})
})