fix(types): make generics with runtime props in defineComponent work (fix #11374) #13119
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
danyadev
force-pushed
the
fix-generic-defineComponent-with-runtime-props
branch
from
cf9276c to
d7d0e0e
Compare
WalkthroughAdjusted a defineComponent overload to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (TSX)
participant API as defineComponent (runtime-core)
participant TS as TypeScript Checker
Note over Dev,API: Define component (maybe generic) + runtime props (array|object)
Dev->>API: call defineComponent(setupFn, { props: [...] / { ... } })
API->>TS: expose component type (uses NoInfer for props keys)
TS-->>API: infer/resolve generic Params and prop typings
Dev->>TS: use component in JSX (with or without explicit generic)
TS-->>Dev: validate props, emit diagnostics or accept usage
Note over TS,Dev: @ts-expect-error annotations validate expected failures
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
danyadev
changed the title
danyadev
force-pushed
the
fix-generic-defineComponent-with-runtime-props
branch
from
d7d0e0e to
5e1f00a
Compare
|
Hi @jh-leong! Rebased the MR on the main branch to pick up all the changes from the released version, upd: it updated the builds, it seems like the pipelines were a bit clogged at the time and didn't show that they were in progress Also updated the description and hopefully made it fresher and easier to understand, plus added the link to the playground! Really need this feature, so I've been using it right from the creation of this MR :) |
| ) => RenderFunction | Promise<RenderFunction>, | ||
| options?: Pick<ComponentOptions, 'name' | 'inheritAttrs'> & { | ||
| props?: (keyof Props)[] | ||
| props?: (keyof NoInfer<Props>)[] |
There was a problem hiding this comment.
One concern, to avoid breaking this case:
defineComponent(
props => {
// Before: { msg: any }
// After : Record<string, any>
// @ts-expect-error: should error when accessing undefined props
props.foo
// auto-completion missing for props
props.msg
return () => {}
},
{
props: ['msg']
}
)We might want to keep the original behavior by adding a new overload instead:
// overload 1: direct setup function
// (uses user defined props interface)
export function defineComponent<
Props extends Record<string, any>,
E extends EmitsOptions = {},
EE extends string = string,
S extends SlotsType = {},
>(
setup: (
props: Props,
ctx: SetupContext<E, S>,
) => RenderFunction | Promise<RenderFunction>,
options?: Pick<ComponentOptions, 'name' | 'inheritAttrs'> & {
props?: (keyof Props)[]
emits?: E | EE[]
slots?: S
},
): DefineSetupFnComponent<Props, E, S>
+export function defineComponent<
+ Props extends Record<string, any>,
+ E extends EmitsOptions = {},
+ EE extends string = string,
+ S extends SlotsType = {},
+>(
+ setup: (
+ props: Props,
+ ctx: SetupContext<E, S>,
+ ) => RenderFunction | Promise<RenderFunction>,
+ options?: Pick<ComponentOptions, 'name' | 'inheritAttrs'> & {
+ props?: (keyof NoInfer<Props>)[]
+ emits?: E | EE[]
+ slots?: S
+ },
+)There was a problem hiding this comment.
I see, but the proposed solution breaks another thing: if runtime props contain more props than declared in props, and the function is generic, the types fall back to any:
(@ts-expect-error is red because there was an error, but it's gone now)
It's still the best solution though, I've tried some variants and none of them worked
There was a problem hiding this comment.
applied your solution for now and added a few tests
There was a problem hiding this comment.
I believe this issue is negligible in practice — it only happens when props is both generic and doesn't match the runtime props, which seems rare.
jh-leong
added
the
🍰 p2-nice-to-have
danyadev
force-pushed
the
fix-generic-defineComponent-with-runtime-props
branch
from
5e1f00a to
b8cc94b
Compare
|
@danyadev sorry for the ping, just wondering what the status of this PR is 🙏 I can see that some tests in the language tools failed |
danyadev
force-pushed
the
fix-generic-defineComponent-with-runtime-props
branch
from
b8cc94b to
4de7ac9
Compare
|
Hi @cernymatej! I've looked through these CI failures and most of them seem to be unrelated. Maybe some snapshots in language-tools need to be regenerated due to a new overload being added, but as a first step I'll suggest just re-running the tests after pulling the main branch Though I can't run CI tests myself, we need a staff member to do so, for example @jh-leong, but he seems to have little activity lately :c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages-private/dts-test/defineComponent.test-d.tsx (3)
1424-1431: Clarify subset relationship between manual props type and runtime keys.The second case is allowed (manual type may be a superset; runtime props narrows accepted external keys). Add an inline comment to avoid confusion with the earlier “must match” wording.
Apply this diff to document intent:
defineComponent( (_props: { msg: string; bar: string }) => { return () => {} }, { + // OK: manual props type can be a superset; runtime-only accepts 'msg' props: ['msg'], }, )
1441-1448: Strengthen the generic-propagation checks for Comp2.Add a local assertion to ensure T flows into setup props, and a TSX case that rejects an explicit wrong generic argument.
const Comp2 = defineComponent( - <T extends string>(_props: { msg: T }) => { - return () => {} - }, + <T extends string>(_props: { msg: T }) => { + // T should flow through, guarding against regression to `any` + expectType<T>(_props.msg) + return () => {} + }, { props: ['msg'], }, ) expectType<JSX.Element>(<Comp2 msg="1" />) expectType<JSX.Element>(<Comp2<string> msg="1" />) // @ts-expect-error msg type is incorrect expectType<JSX.Element>(<Comp2 msg={1} />) +// @ts-expect-error explicit wrong generic argument violates `T extends string` +expectType<JSX.Element>(<Comp2<number> msg={1} />) // @ts-expect-error msg is missing expectType<JSX.Element>(<Comp2 />) // @ts-expect-error bar doesn't exist expectType<JSX.Element>(<Comp2 msg="1" bar="2" />)Also applies to: 1469-1476
1460-1467: Mirror the subset clarification for generic case.Like the non-generic case, consider an inline comment noting that manual props may be a superset while runtime narrows accepted keys.
defineComponent( <T extends string>(_props: { msg: T; bar: T }) => { return () => {} }, { + // OK: manual includes 'bar', but runtime only accepts 'msg' props: ['msg'], }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages-private/dts-test/defineComponent.test-d.tsx(4 hunks)packages/runtime-core/src/apiDefineComponent.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-core/src/apiDefineComponent.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages-private/dts-test/defineComponent.test-d.tsx (1)
packages/runtime-core/src/apiDefineComponent.ts (1)
defineComponent(305-315)
🔇 Additional comments (7)
packages-private/dts-test/defineComponent.test-d.tsx (7)
1405-1412: Good positive coverage for array runtime props (non-generic).Comp1 and its TSX assertions correctly validate the happy path and guard against “any” regressions via the wrong-type and missing-prop cases.
Also applies to: 1433-1440
1414-1422: Negative case for extra runtime keys is correct.This catches the scenario where options.props includes a key not present in the manual props type.
1450-1458: Correct negative case for extra runtime keys with generics.This ensures NoInfer keeps inference from the array from corrupting the generic props type.
1478-1488: Object runtime props: limitation is well captured.Tests clearly encode that generics aren’t supported with object-form runtime props and that explicit type arguments should error in TSX.
Also applies to: 1514-1523
1491-1501: Negative cases for object runtime props are correct.Both “missing in manual type” and “generic + object props” error scenarios are asserted properly.
Also applies to: 1503-1512
1525-1532: Name mismatch test looks good.Covers the “string prop names don’t match” edge.
1534-1544: Type mismatch test looks good.Asserting Number vs string type mismatch prevents silent widening.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
danyadev
force-pushed
the
fix-generic-defineComponent-with-runtime-props
branch
from
4de7ac9 to
5571da1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages-private/dts-test/defineComponent.test-d.tsx (2)
1442-1448: Strengthen generic preservation with a direct assertion inside setupTo ensure
Tisn’t widened (i.e., not inferred asany) when array runtime props are present, add a local check that_props.msgis exactlyT.Apply this minimal diff:
const Comp2 = defineComponent( <T extends string>(_props: { msg: T }) => { + expectType<T>(_props.msg) return () => {} }, { props: ['msg'], }, )
1450-1458: Negative generic cases look right; add one constraint error for completeness
- The “extra key” and “missing in runtime” checks are on point.
- Optional: add an explicit generic-constraint failure to prove TS won’t allow
<number>forT extends string.You can extend the TSX assertions near Comp2 with:
expectType<JSX.Element>(<Comp2 msg="1" />) expectType<JSX.Element>(<Comp2<string> msg="1" />) + // @ts-expect-error T extends string, number is not allowed + expectType<JSX.Element>(<Comp2<number> msg={1} />)Also applies to: 1460-1468
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages-private/dts-test/defineComponent.test-d.tsx(4 hunks)packages/runtime-core/src/apiDefineComponent.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-core/src/apiDefineComponent.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages-private/dts-test/defineComponent.test-d.tsx (1)
packages/runtime-core/src/apiDefineComponent.ts (1)
defineComponent(305-315)
🔇 Additional comments (3)
packages-private/dts-test/defineComponent.test-d.tsx (3)
1405-1413: Array runtime props with manual types: good coverage and intent is clear
- Positive and negative cases (extra key, missing key, wrong TSX prop type) look correct and align with the NoInfer behavior you’re enforcing. No issues spotted.
Also applies to: 1414-1423, 1424-1432, 1433-1440
1479-1488: Explicitly documenting “object runtime props + generics = unsupported” is valuable
- The tests correctly assert that generics aren’t supported with object-format runtime props, and the negative cases (missing keys, explicit generic usage) are precise.
- No changes requested.
Also applies to: 1491-1500, 1502-1512, 1514-1523
1524-1533: Mismatched names and type-mismatch checks are precise
- Great edge coverage for incorrect key names and validator-type mismatches.
Also applies to: 1535-1544
|
Hi @edison1105, can you please approve the ci workflow to run pkg-pr-new? That way I'll be able to grab the latest vue version with this MR's code included Though it would be much better to make progress with this MR, and your help would be much appreciated. We stumbled on some ecosystem CI failures because of the changes in TS declarations. To fix them, we need to update affected .d.ts files, but I don't know how to do it correctly across the repositories upd: published the package myself: https://pkg.pr.new/danyadev/vuejs-core/vue@37fda96 |
|
This PR fixes both issues, because it introduces support for passing generics to components too. The second issue was created after this PR was created, so I didn't know about it. I'll mark it to be fixed too, thanks |
6 participants
fixes #13763, fixes #11374
relates to #12761 (comment), #7963 (comment)
Problem
The "support" for generics in defineComponent was introduced in #7963, but it simply doesn't work: when you pass the
propsoption, TypeScript ignores the main props definition and infers the props type from thepropsoption instead.Unfortunately, this option doesn't provide any useful type hints except for the names of the props, so all the props become
anyHere is a simple example, where instead of expected normal types we encounter
any:https://play.vuejs.org/#eNp9Ustu2zAQ/JUFL1IAQ0bR9qLKAtogh/bQBIlvYRAY0lphQi8FklIcCPr3LCnLeSIniTO7s7OPQfxu26zvUOSicJVVrQe9oWYlhXd7KUpJatca62GAGreK8NTwm5A8jLC1ZgcJZye/JEnyTy3ChTWtK9YlrGCQBOBQY+WxzmEdnoZO71gfc0hN65Uhxk9gVUJvVC1pDDqVIefhKiayzLu6abEG3Huk2oHzVlFTpm0omh9rR8FY3aLvLEEakaJWffniZ4hZ2QyMxTLw7GEx5R5Er5M5IllAMvtPbjjwJLjFfZwPu9x0On7fuJ1KfzSTBgQmT9MvPw49zwV5C1tjpDhObTWkMxdFwqSMxkyb5oUYYXlQnDsCYKfBbbGcdsyYWPCGOX+rmuzeGeIDiCalqNi70mjP436cFDyqSU+Kjdbm8V/EvO1wMePVHVYPn+D34Yhy/rmw6ND2KMWR8xvboJ/os6v/vNNX5M7UneboL8hL5N674HEK+9NRzbZfxUW3f+P98pms3Vk4Gzc3FYyGSL65kadx26MNHA/ie/Yz+/ZDjM+7YwkP
Solution
Let's look at one of the overloads of defineComponent:
Here we can see the
Propsgeneric type, thesetupargument usingPropsand theoptionsargument also usingPropsWhen we add a generic type to the
setupfunction, it becomes less obvious for TypeScript to decide which variable to use to infer the generic type, and unfortunately for us it chooses the variant with less type density.So we need to tell TypeScript not to infer
Propsfrom theoptions.propsfield:Another solution
Initially I've come up with another solution which doesn't rely on that new TypeScript
NoInfertype. But as you already useNoInferin runtime code, it may be irrelevant.It works by separating
Propsinto two generics — originalPropsandDeclaredProps— and using them differently:A note about an object format for props with runtime validations
This MR fixes only the usage with props defined as an array of strings. I haven't found any solution for the object format and I'm not sure that there is one...
But on the other side, I don't think someone would need to combine generics with runtime validations
Summary by CodeRabbit
Tests
New Features