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

fix: vm should be properly typed #63

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Conversation

cexbrayat
Copy link
Member

@cexbrayat cexbrayat commented Apr 13, 2020

This is on top of #64

The types have been tweaked to avoid the need of casting like (wrapper.vm as any).msg.

The test showcases the correct behavior: you can reproduce the issue by adding the test to the master banch. It will:

  • not compile without a as any cast
  • fail as it returns the vm of the parent which has no msg, isEnabled fields and toggle function.

@cexbrayat cexbrayat changed the title fix: vm should be the vm of the tested component fix: vm should be properly typed Apr 13, 2020
@cexbrayat
Copy link
Member Author

I splitted the two fixes in 2 separate PRs:

@cexbrayat cexbrayat force-pushed the fix/vm branch 2 times, most recently from 5be170c to 12d1929 Compare April 14, 2020 05:00
@lmiller1990
Copy link
Member

lmiller1990 commented Apr 14, 2020

Hey, thanks for helping out! Typing this correctly is something I am struggling with. I don't think this is entirely correct, though. wrapper needs to be able to accept objects like :

const Foo = {
  template: '<div>Foo</div>'
}

Not everyone will use defineComponent. With this change, that won't work.

image

Full error:

const App: {
    render(): VNode<RendererNode, RendererElement>;
}
Argument of type '{ render(): VNode<RendererNode, RendererElement>; }' is not assignable to parameter of type 'new () => ComponentPublicInstance<{}, {}, {}, {}, {}, {}, {}>'.
  Type '{ render(): VNode<RendererNode, RendererElement>; }' provides no match for the signature 'new (): ComponentPublicInstance<{}, {}, {}, {}, {}, {}, {}>'.ts(2345)

🤔

@lmiller1990 lmiller1990 self-requested a review April 14, 2020 11:50
@cexbrayat
Copy link
Member Author

I get your point. Maybe I can simply add an override, so people with defineComponent will have a proper typing and others will be at the same point as now. Most TS users are going to use it anyway, otherwise you don't have proper typings in your components, and we can probably improve later.

The types have been tweaked to avoid the need of casting like `(wrapper.vm as any).msg`.
@cexbrayat
Copy link
Member Author

@lmiller1990 PTAL: I added a simple override instead of changing the signature. So everything works the same, but, if you use defineComponent, you get proper typings of vm

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

one comment/question, thanks!

export function mount<T extends any>(
originalComponent: any,
options?: MountingOptions
): VueWrapper<any>
Copy link
Member

Choose a reason for hiding this comment

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

thanks @cexbrayat I will give this a quick test tmr and get it merged.

While we are here, can we get typings in the props mounting options? I was able to do that in an experiment I built prior to this, found here

Eg then we could do

mount(Foo, {
  props: { 
    bar: 'sadf' // <= can we get typesafety here? that would be cool as
  }
})

If this is difficult to accomplish don't worry about it too much for now, but I agree, we should provide as many typings as possible.

Copy link
Member

Choose a reason for hiding this comment

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

eg: component: new () => ComponentPublicInstance<P> (or something)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll try to look into it soon, and I'll open a dedicated PR 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

@lmiller1990 FYI I added some typings tests using tsd in #72 , improved the typing of vm some more in #73 and started working on properly typings the props options in #74

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 15, 2020

Awesome, love it. TS changed my life.

I will do the next alpha when the oustanding PRs are merged in and the new Vue alpha comes out since we are waiting on a few bugfixes there.

@lmiller1990 lmiller1990 merged commit 44b5f48 into vuejs:master Apr 15, 2020
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.

2 participants