chore(findComponent): refactor find & findComponent (fixes #716, #899) #1094
Conversation
xanf
force-pushed
the
improve-find
branch
3 times, most recently
from
7c70668 to
977363e
Compare
| name: string | ||
| length?: never |
There was a problem hiding this comment.
This was a nasty one. { name: string } definition matches Function (because functions have name property) which messes with our type inference. Putting length to never allows us to distinguish between finding a function and finding by object with name field
There was a problem hiding this comment.
damn, nasty but smart. thanks for the explanation, I would've missed it
| 'NestedChild' | ||
| ) | ||
| expect( | ||
| wrapper.findAllComponents<DefineComponent>('.in-root')[0].vm.$options.name |
There was a problem hiding this comment.
I like that when you're doing something nasty (working outside WrapperLike interface which is unified across VueWrapper and DOMWrapper - you need to explicitly specify "hey, I know that I'm doing, there will be no functional component here". Type safety as it is!
xanf
force-pushed
the
improve-find
branch
2 times, most recently
from
572f3b6 to
c795b0d
Compare
| T extends Omit< | ||
| ComponentPublicInstance, | ||
| '$emit' | keyof ComponentCustomProperties | ||
| > & { | ||
| $emit: (event: any, ...args: any[]) => void | ||
| } & ComponentCustomProperties = ComponentPublicInstance | ||
| > extends BaseWrapper<Node> { |
There was a problem hiding this comment.
TypeScript as usually sucks in co- and contravariance, that's why we need to relax check on $emit a bit (in other words $emit generated by InstanceType<DefineComponent<...>> which is ComponentPublicInstance<...> is not a subclass of ComponentPublicInstance (with default params) due to params type mismatch in EmitFn (the code of this helper type in vue-next is a bit insane)
But I wish someone told me why i need to decompose & put back ComponentCustomProperties - or mount will fail typecheck otherwise 😕
|
@cexbrayat will be great to have another pair of eyes, especially on TypeScript typings :) @lmiller1990 it seems that I've occasionally implemented circuit-breaker similar to your one at 2700e88, so this commit kinda reverts that change, sorry (I think separate |
* allow find also find DOM nodes by ref * generalize findAll/findAll components behavior * greatly improve typings * add types tests for findComponent
| ComponentPublicInstance, | ||
| '$emit' | keyof ComponentCustomProperties | ||
| > & { | ||
| $emit: (event: any, ...args: any[]) => void |
There was a problem hiding this comment.
Should this be event: string?
There was a problem hiding this comment.
Unfortunately no. ComponentPublicInstance<> (with all default parameters) infers $emit exactly as (event: string, ...args: any[]) => void. When you pass here a component with defined emits (for example hi) $emit will be (event: "hi") => void' which is not a subtype of (event: string, ...args: any[]) => void (event argument is too narrow). That's why we need to cheat with any
I've tried to add event: EmitType and EmitType extends string = string as parameter to VueWrapper generic to express this limitiation , but it seems typescript still infers wrong type
There was a problem hiding this comment.
Nice work! To be honest, it's hard to review, but LGTM overall.
@pikax maybe you can take a look for the types part?
@lmiller1990 As this might introduce unwanted regressions/behavior differences, maybe we could release it on a special tag, before making it rc.18?
Sorry for that, I promise not to do such big one in future 🤗. It was unexpectedly too interconnected
I would like not to do that. We have relatively small user base, so adding a special tag will not help that much, making sure it is even less tested. I've ran this change accross big suite (2k+ tests) with no changes, so I'm pretty confident in it except types part :)
Sorry. I totally overlooked this one. I pushed separate commit e5e42dc to fix warning, WDYT of that? |
Since vuejs#1094, find supports element refs. This PR adds the new use case to the docs.
Since #1094, find supports element refs. This PR adds the new use case to the docs.
Sorry for such a big one, I was working on typings and improving typings revealed multiple issues
Basically major changes in this PR are:
findnow finds DOM nodes by ref (Supportfind({ ref })syntax #716)findAllComponents/findComponent/getComponentmigrated tobaseWrappersince they are now officially supported in both wrappersfindComponent/findAllComponentswith better type inference by large margin.element- it was originallyElementwhich is actually wrong - it could be Element, Comment (when rendering empty v-if), Text (if component renders just raw text). Fixed this typings and added guards everywherefindComponentandfindAllComponentsare now silently returning DOMWrappers for functional components encountered. This mimics VTU v1 behavior and really helps with migration