feat(runtime-core): mixins/extends support in TypeScript #626
Conversation
dolymood
changed the title
|
Since this is a pretty substantial PR, please be patient it takes some time for us to review it. |
| mixins?: LegacyComponent[] | ||
| extends?: LegacyComponent | ||
| mixins?: Mixin[] | ||
| extends?: Mixin |
There was a problem hiding this comment.
extends can't be Mixin because if this typing enforces extends to be from type declared in the mixin
// works
mixins: [mixinA, mixinB, mixinC],
extends: mixinB,
//doesn't work
mixins: [mixinA, mixinB],
extends: mixinC,
|
@pikax Thanks for your first review. I've merged vue-next master codes and fixed |
| expect(this.c).toBe(3) | ||
| // should not visit other props/data... in one mixin | ||
| // ????? only test current mixin props/data | ||
| // expect(this.b).toBe(2) |
There was a problem hiding this comment.
This test is quite important, because is expected you to access the parent props in the mixin.
But with typed mixins it makes quite hard to get those types using defineComponent(maybe another api defineMixin<vmProps>?).
I think this test would be better if you remove defineComponent from the mixins and call it in the Comp mixin
const mixinA = {
// ...
}
const mixinB = {
//...
}
// etc
const Comp = defineComponent({
mixins: [
defineComponent(mixinA),
defineComponent(mixinB),
defineComponent(mixinC)
],
// etc
)Or having it has two tests, one defining typed based other using non-typed (no-defineComponent)
There was a problem hiding this comment.
Very good suggestion！ 👍
Mixin types was tested in defineComponent-test-d.tsx for types cases.
Here we must ensure the runtime logic is correct.
|
|
||
| expect(renderToString(h(Comp))).toBe(`12`) | ||
| expect(calls).toEqual(['base', 'comp']) | ||
| }) | ||
|
|
||
| test('extends with mixins', () => { |
There was a problem hiding this comment.
Same as the previous component, we probably want to test the access of the component who's extending.
|
@dolymood Thank you for the changes! It looks good so far (went through most of the code), but unfortunately the current typings for Types returned by ComponentPublicInstanceConstructor<
CreateComponentPublicInstance<
ExtractPropTypes<PropsOptions>,
RawBindings,
D,
C,
M,
Mixin,
Extends,
E,
VNodeProps & ExtractPropTypes<PropsOptions, false>
>
> &
ComponentOptionsWithObjectProps<
PropsOptions,
RawBindings,
D,
C,
M,
Mixin,
Extends,
E,
EE
>Will be
Manage to make it build & pass all the tests by assigning an interface to the I think we should bump to TS 3.9 before we merge this PR :/ Sorry @dolymood to keep asking you to change this PR 🙏 Suggestions for 3.9 are very welcome :) |
|
Bump to TS 3.9 is very necessary. I will try to update codes to compatible with TS 3.9. |
# Conflicts: # packages/runtime-core/src/apiDefineComponent.ts
|
Regarding TS 3.9 issue, looks like we are affected by "Intersections Reduced By Discriminant Properties" breaking change. Looks like the culprit property is I'm not sure how this property type work in Vue typings but changing its type to bottom function type seems to solve the issue. call?: (this: unknown, ...args: unknown[]) => never |
This reverts commit 11cd53d. # Conflicts: # packages/runtime-core/src/apiDefineComponent.ts
# Conflicts: # yarn.lock
# Conflicts: # yarn.lock
We want to support
mixins/extendsoptions in TS.Include changes:
defineComponent:{new(): ComponentPublicInstance}->{new(): ComponentPublicInstance} & (ComponentOptionsWithoutProps| ComponentOptionsWithArrayProps| ComponentOptionsWithObjectProps)defineComponentandComponentOptionsWithoutProps,ComponentOptionsWithArrayProps,ComponentOptionsWithObjectPropsandComponentOptionsBase, LegacyOptions: add new type variableMixinwhich should be defined bydefineComponenth(): splith(type: ComponentOptionsWithoutProps|ComponentOptionsWithArrayProps)becauseComponentOptionsWithArrayProps/ComponentOptionsWithObjectProps extends ComponentOptionsWithoutPropswill betrue. If thepropscan not match theComponentOptionsWithArrayProps, it will be match theComponentOptionsWithoutPropscases with error type props.