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.

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(types): add typescript declarations #15

Merged
merged 6 commits into from
Jul 17, 2017
Merged

feat(types): add typescript declarations #15

merged 6 commits into from
Jul 17, 2017

Conversation

ktsn
Copy link
Member

@ktsn ktsn commented Jul 17, 2017

This PR adds TypeScript declarations of vue-test-utils.
I wrote these declarations by reading the current docs and implementation but not sure that the declaration is correct. So please let me know if somethings are wrong 😉

It would be easier to confirm by seeing test files under types/test. They include actual usage of vue-test-utils API in TypeScript.

@HerringtonDarkholme It would be appreciated if you can review this PR 🙂

Directory structure

  • types/
    • index.d.ts All declarations of vue-test-utils are in this file. This should be published in npm.
    • tsconfig.json TS compiler options are defined in this file
    • test/ includes test files for typings

Allow annotating type for mount/shallow

One of challenge when using vue-test-utils would be how to declare wrapper.vm to an actual component type. For example, if we simply declare it as Vue, we cannot test component properties/methods since they are not declared in Vue.

@Component
class SomeComponent extends Vue {
  foo = 'bar'
}

const wrapper = mount(SomeComponent)
assert(wrapper.vm.foo === 'bar') // This throws a compile error

To solve this, I declared mount/shallow type that can be receive a component type via generics.

@Component
class SomeComponent extends Vue {
  foo = 'bar'
}

const wrapper = mount<SomeComponent>(SomeComponent)
assert(wrapper.vm.foo === 'bar') // This passes compiler type checking

const anyWrapper = mount<any>(SomeComponent) // We can use `any` type if we don't have appropriate type
assert(wrapper.vm.someOtherProperty === 'baz')

// Wrapper#find and Wrapper#findAll can receive a type parameter as well
const found = wrapper.find<SomeComponent>(SomeComponent)
const list = wrapper.findAll<SomeComponent>(SomeComponent)

Added npm script to test typings

I've added npm run test:types to run the typings test. It also be included in npm run test.

@ktsn
Copy link
Member Author

ktsn commented Jul 17, 2017

Rethinking about WrapperArray#at, I guess it would be better that WrapperArray itself has a type parameter since its items always are the same type.

types/index.d.ts Outdated
readonly element: HTMLElement
readonly options: WrapperOptions

find<R extends Vue> (selector: Selector): Wrapper<R>

Choose a reason for hiding this comment

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

We can separate find to more overloadings. Thus for VClass and CompoentOption there is no need to manually cat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out.
By the way, I realized a type parameter could not be inferred from other type parameter so we have to manually set a component type if we use component constructors. Does this impossible until microsoft/TypeScript#14400 is solved, right?

// find<R extends Vue, Ctor extends VueClass<R> = VueClass<R>> (selector: Ctor): Wrapper<R>

// `find`'s type parameters will be `<Vue, typeof ClassComponent>`
const found = wrapper.find(ClassComponent)

Choose a reason for hiding this comment

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

Exactly. VueJS really needs it.

attachedToDocument: boolean
}

interface MountOptions<V extends Vue> extends ComponentOptions<V> {

Choose a reason for hiding this comment

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

I wonder the status of vuejs/vue#5887.

That PR will break this declaration. But 5887 does not seem to have activities...

@ktsn @yyx990803 What's your opinion?

I can change 5887 to have a compatible version by changing ComponentOptions, though. @DanielRosenwasser

Copy link
Member Author

Choose a reason for hiding this comment

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

I think vuejs/vue#5887 will not be merged soon since we need to have some migration time for the new declaration. So we can use the current declaration for now.

interface MountOptions<V extends Vue> extends ComponentOptions<V> {
attachToDocument?: boolean
clone?: boolean
context?: VNodeData

Choose a reason for hiding this comment

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

This is only used by FunctionalComponent, can we make two separate options for different usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that but I ended up to choose type declaration simplicity. Also I have no idea to declare if a constructor of functional component is passed to mount/shallow 🤔

types/index.d.ts Outdated
* If it is an array of string, the specified children are replaced by blank components
*/
type Stubs = {
[key: string]: Component | string | boolean

Choose a reason for hiding this comment

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

types/index.d.ts Outdated
localVue?: typeof Vue
intercept?: object
slots?: Slots
stub?: Stubs

Choose a reason for hiding this comment

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

I wonder why this API is called stub rather than stubs, since it can stub multiple components? @eddyerburgh

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right, stubs is more appropriate 👍

Copy link
Member

Choose a reason for hiding this comment

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

@ktsn I've renamed stub to stubs in 0ff0f63

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated 👍

clone?: boolean
context?: VNodeData
localVue?: typeof Vue
intercept?: object
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme Jul 17, 2017

Choose a reason for hiding this comment

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

@eddyerburgh I wonder why intercept is by default global.

https://github.com/vuejs/vue-test-utils/blob/master/docs/en/api/mount.md

if (options.intercept) {

I know this option is used to be global. But since it is changed its, it seems better to modify localVue by default. Since one mount returns one wrapper, it feels more natural that intercept only modify the wrapper.

If users want to modify Vue's prototype globally, we can, if strongly desired, provide another global API like mount. But fairly users can just Vue.install by themselves.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't add them globally, it's just a poorly named function. I'll refactor to make that clear

Copy link
Member

Choose a reason for hiding this comment

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

I've refactored in 086f8ae

@eddyerburgh eddyerburgh merged commit 86c8a7e into vuejs:master Jul 17, 2017
@ktsn ktsn deleted the add-types branch July 17, 2017 08:40
@snaptopixel
Copy link

@ktsn and @HerringtonDarkholme are heros, just sayin'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants