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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdjusted a defineComponent overload to use NoInfer for prop keys to avoid eager inference, and expanded DTS tests with named function-syntax components (Comp1/Comp2/Comp3) covering array vs object runtime props, generics, JSX usage, and expected TypeScript errors. 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: provide component type (uses NoInfer on props keys)
TS-->>API: infer generic parameters and prop typings
Dev->>TS: use component in JSX (with/without explicit generics)
TS-->>Dev: validate props, emit diagnostics or accept usage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
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 |
|
Any update on this PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages-private/dts-test/defineComponent.test-d.tsx (1)
1469-1476: Strengthen the explicit-generic assertion.Line 1470 only proves that
<Comp2<string> />is accepted. It would still pass if the explicit type argument were ignored andmsgstayed plainstring. Since this PR is also meant to cover the TSX type-argument path, please add a narrower instantiation so the test fails unless the type argument is actually honored.Suggested test tightening
expectType<JSX.Element>(<Comp2 msg="1" />) - expectType<JSX.Element>(<Comp2<string> msg="1" />) + expectType<JSX.Element>(<Comp2<'1'> msg="1" />) + // `@ts-expect-error` explicit generic should narrow accepted values + expectType<JSX.Element>(<Comp2<'1'> msg="2" />) // `@ts-expect-error` msg type is incorrect expectType<JSX.Element>(<Comp2 msg={1} />)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages-private/dts-test/defineComponent.test-d.tsx` around lines 1469 - 1476, The explicit-generic test for Comp2 should use a narrower type argument so the compiler must honor the type parameter; replace the current generic case (expectType<JSX.Element>(<Comp2<string> msg="1" />)) with a literal/narrow instantiation that only accepts that exact value (for example use a string-literal type like Comp2<'special'> with msg="special") so the test fails if the TSX type argument is ignored; update the expectType line referencing Comp2 accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages-private/dts-test/defineComponent.test-d.tsx`:
- Around line 1424-1431: The test currently uses defineComponent where the setup
props type includes "bar" but options.props omits it, so the positive case only
asserts absence of extra runtime keys rather than full type/options equivalence;
update the tests to make them true positives by adding "bar" to the
options.props array (i.e., ensure options.props includes both 'msg' and 'bar'
for the defineComponent call(s)), or alternatively soften the test description
to explicitly state the current limitation—apply this change for both
occurrences that reference the setup props vs options.props mismatch.
---
Nitpick comments:
In `@packages-private/dts-test/defineComponent.test-d.tsx`:
- Around line 1469-1476: The explicit-generic test for Comp2 should use a
narrower type argument so the compiler must honor the type parameter; replace
the current generic case (expectType<JSX.Element>(<Comp2<string> msg="1" />))
with a literal/narrow instantiation that only accepts that exact value (for
example use a string-literal type like Comp2<'special'> with msg="special") so
the test fails if the TSX type argument is ignored; update the expectType line
referencing Comp2 accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce37b553-c1dd-4c0f-abac-072edf144071
📒 Files selected for processing (2)
packages-private/dts-test/defineComponent.test-d.tsxpackages/runtime-core/src/apiDefineComponent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-core/src/apiDefineComponent.ts
| defineComponent( | ||
| (_props: { msg: string; bar: string }) => { | ||
| return () => {} | ||
| }, | ||
| { | ||
| props: ['msg'], | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The new positive cases don't actually verify a full "match".
Lines 1424-1431 and 1460-1467 compile even though bar exists in the setup props type but is missing from options.props. That means this block is only asserting “no extra runtime keys,” not bidirectional equivalence. Please either call out that limitation here or soften the section wording so the test does not document a stronger guarantee than the overload currently enforces.
Also applies to: 1460-1467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages-private/dts-test/defineComponent.test-d.tsx` around lines 1424 -
1431, The test currently uses defineComponent where the setup props type
includes "bar" but options.props omits it, so the positive case only asserts
absence of extra runtime keys rather than full type/options equivalence; update
the tests to make them true positives by adding "bar" to the options.props array
(i.e., ensure options.props includes both 'msg' and 'bar' for the
defineComponent call(s)), or alternatively soften the test description to
explicitly state the current limitation—apply this change for both occurrences
that reference the setup props vs options.props mismatch.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
7 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