-
Notifications
You must be signed in to change notification settings - Fork 272
feat: improve mount typings when using object syntax and infer Data type #110
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked some questions, this is pretty based
Maybe @cexbrayat can take a quick look 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a nice improvement 👍
I added several comments. I think it would be great:
- to have more tests in
mount.d.ts
for the functional component and for the data option cases - to either simplify the signatures if possible, or comment/test if we can't simplify to explain why
Ping me when you're done and I'll take another look!
src/mount.ts
Outdated
ctx: SetupContext | ||
) => RawBindings | RenderFunction, | ||
|
||
options: MountingOptions<Props, {}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: as {}
is the default value for the Data
type, I think you can simplify to MountingOptions<Props>
?
src/mount.ts
Outdated
{}, | ||
// public props | ||
VNodeProps & Props | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for handling functional component. Can you add tests in mount.s.ts
for this please?
OOC: do we need to declare all the generics here? If we can achieve what we want with just ComponentPublicInstance<Props>
? If so, I vote to keep it simple.
If we can't, I would love to:
- have a comment to explain why
- have a tsd test that fails without it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the functional overload can be removed in favour of the
// Functional component
export function mount<
TestedComponent extends FunctionalComponent<Props>,
Props
>(
originalComponent: TestedComponent,
options?: MountingOptions<Props>
): VueWrapper<ComponentPublicInstance<Props>>
C extends ComputedOptions = {}, | ||
M extends Record<string, Function> = {}, | ||
E extends EmitsOptions = Record<string, any>, | ||
EE extends string = string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: if we really need all the generics, it would be great to have a test showcasing why.
Also, maybe pick more explicit names for the generic types and add comments: I struggle to follow what they refer to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is basically a copy&paste
from defineComponent.ts
To make it simpler to maintain (bring the types from that file), I can add a comment explaining.
mount(AppWithDefine, { | ||
props: { a: 'Hello', b: 2 } | ||
}).vm.a | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the test is more readable when splitted like it was. Same for the following changes.
mount( | ||
{ | ||
props: ['a'] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to see that it throws an error, but I don't get how this is different from the test case with AppWithArrayProps
which still succeeds? Maybe add more explanation in the comment.
Ideally, shouldn't mount(AppWithArrayProps, { props: { a: 'Hello', b: 2 } })
also fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically when you use mount({ props: ['a']}) is basically the same as doing
defineComponent({props:['a']}), the ThisType will be inferred as the
$vm`, resulting in:
mount({
props: ['a']
})
// type
type MountTyped = {
props: ['a']
}
const Comp = {
props: ['a']
}
//type
type CompTyped = {
props: string[]
}
Basically using mount/defineComponent
we maintain the actual tuple value, when using a variable with the options, typescript will resolve the tuple as string[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! So the previous test should also throw if the props were declared as const right?
const AppWithArrayProps = {
props: ['a'] as const,
template: ''
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem to happen, if you define as const
, it will use props: {}
overload 🤔
} | ||
} | ||
}) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I think more tests with data would be awesome. Can we pass data
to all type of components? Can we pass data that are not declared? etc. That will greatly help us maintining the lib if we have extensive coverage of the different cases.
Is this one awaiting more tests? If so, I can try and help out. LMK where to start - just go over the comments and add those tests? |
@lmiller1990 Yes! I don't know if @pikax had time to start working on them... If not and if you are willing to add some, I'll gladly review them 👍 |
Have a few other things to work on atm (mainly docs, and vue-jest). If we don't have any bandwidth to add these tests right now, rather than let this PR hang around forever, I'll rebase and get it merged. Is there any must have tests we don't have? We do have some basic ones for mount and shallow. |
Should we merge or do you guys want to add more tests before that? |
I thought about this a bit more. As cool as this feature is, until we have some way to actually get the types from a |
I'm OK with merging it. I think it's still better to have it than not, even if I agree with you @lmiller1990 |
I think we should definitely have them towards the end of beta and moving into a release candidate - for now I'm happy to merge this up and revisit when we have better IDE integration. I am really quite excited about this - I know little to nothing about language servers, might be time to learn. |
This improves typing when using
and also adds the data type validation