-
-
Notifications
You must be signed in to change notification settings - Fork 9k
fix(runtime-core): keep options API typing intact when expose is used #14118
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
fix(runtime-core): keep options API typing intact when expose is used #14118
Conversation
WalkthroughChanges modify TypeScript type definitions in the Options API component setup to fix a typing regression where using the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
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: |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages-private/dts-test/defineComponent.test-d.tsx (1)
2111-2144: New #14117 dts test repros the regression clearlyThis
defineComponentblock is a good minimal dts-level repro:computed1touches setup bindings (both exposed and non-exposed), data, props, and methods whileexpose: ['setup1']is present, so any future regression that narrowsthisto the expose keys will break compilation here. If you want even stronger guarantees, you could optionally addexpectTypeassertions onthis.setup1,this.setup2, etc., but the current form is already effective.Please ensure this file is included in the dts test run used in CI so this scenario remains covered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages-private/dts-test/defineComponent.test-d.tsx(1 hunks)packages/runtime-core/src/apiDefineComponent.ts(1 hunks)packages/runtime-core/src/componentOptions.ts(2 hunks)
🧰 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test / e2e-test
- GitHub Check: test / unit-test-windows
🔇 Additional comments (2)
packages/runtime-core/src/apiDefineComponent.ts (1)
258-277: Decoupling Options APIthisfromexposelooks correct and localizedRouting the
CreateComponentPublicInstanceWithMixinsExposedparameter tostringin theThisType<...>for the options-object overload ensures thatexposeno longer shrinks thethistype inside options (data/computed/methods/etc.), while the returnedDefineComponentstill threads the realExposedthrough to the public instance type. This is the right place to fix #14117 and should not affect external instance typings or the existingexposetests, since only the internal optionsthiscontext is relaxed here.Please confirm the dts suite / vue-tsc fixtures still pass (
packages-private/dts-test) so we’re sure no other call sites relied on the previous tighterExposedwiring forThisType.packages/runtime-core/src/componentOptions.ts (1)
1181-1199: Aligning legacy ComponentOptionsThisTypewith theexposefix is appropriate*Passing a plain
stringas theExposedparameter toCreateComponentPublicInstanceWithMixinsin these deprecatedComponentOptionsWithoutProps/ComponentOptionsWithArrayPropsaliases makes theirthis-context behavior consistent with thedefineComponentoverload fix:exposecontinues to control the public instance surface but no longer restricts the optionsthistype. This improves ergonomics for users still on these aliases and should not affect existing “expose typing” tests, since the public instance type still carries the realExposedunion.Please re-run the dts tests that exercise the legacy
ComponentOptions*exports to confirm there are no unexpected downstream type changes.Also applies to: 1243-1261
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
Fixed: #14117
Fixed: vuejs/language-tools#5069
Summary by CodeRabbit
Type System Changes
Tests