fix(nuxt): compatibility of lazy component types and h
#33046
Conversation
|
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
WalkthroughThe pull request changes TypeScript type aliases in packages/nuxt/src/components/templates.ts by reordering intersections for two types: IslandComponent and LazyComponent so the DefineComponent part precedes & T. It exports these types via the #components module and adds/updates type-checking tests in test/fixtures/basic-types/app/app-types.ts to validate event prop typings for island and lazy components. No runtime behaviour changes are introduced. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nuxt/src/components/templates.ts (1)
135-135: ConstrainTand document the rationale to prevent regressionsRecommend constraining
TtoDefineComponent(as done forIslandComponent) and adding a short comment explaining why theDefineComponentpart must come first. This guards accidental misuse and prevents future “helpful” reorderings.Apply this diff:
- type LazyComponent<T> = DefineComponent<HydrationStrategies, {}, {}, {}, {}, {}, {}, { hydrated: () => void }> & T + /** + * Keep DefineComponent on the left: improves Vue `h()`/JSX contextual typing so props/emits + * from the original component are preserved (see PR #33046). + */ + type LazyComponent<T extends DefineComponent = DefineComponent> = + DefineComponent<HydrationStrategies, {}, {}, {}, {}, {}, {}, { hydrated: () => void }> & T
📜 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 (1)
packages/nuxt/src/components/templates.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/components/templates.ts
🧠 Learnings (1)
📚 Learning: 2024-11-05T20:04:39.622Z
Learnt from: Tofandel
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/runtime/client-delayed-component.ts:62-62
Timestamp: 2024-11-05T20:04:39.622Z
Learning: In Vue components, when typing the `hydrate` prop for hydration strategies like `hydrateOnInteraction`, use `type: null as PropType<Arguments<typeof hydrateOnInteraction>[0]>` to get the type directly from Vue's hydration methods. Also, remember that `HTMLElementEventMap` is not a type but an object; use `keyof HTMLElementEventMap` when defining prop types for events.
Applied to files:
packages/nuxt/src/components/templates.ts
⏰ 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: build
- GitHub Check: code
🔇 Additional comments (1)
packages/nuxt/src/components/templates.ts (1)
135-135: Intersection order fix restores properh()inference — LGTMSwitching to
DefineComponent<…> & Tfixes the widening of callback params (e.g.onClick’s argument no longer becomesany) when used withh(). This aligns with how TypeScript performs contextual typing over intersections for Vue component types.
CodSpeed Performance ReportMerging #33046 will not alter performanceComparing Summary
|
|
@KazariEX would you be able to have a look at the type test? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/fixtures/basic-types/app/app-types.ts (1)
406-423: Harden lazy component event typing and (optionally) assert built-in emits.
- Mirror the stricter checks so we catch regressions where
foobecomesany.- Optional: since
LazyComponentdeclares an emittedhydratedevent, add a quick check foronHydratedprops.Apply these changes:
h(LazyComp, { onClick: (foo) => { - foo satisfies string + // Ensure the parameter is strictly typed + expectTypeOf(foo).toEqualTypeOf<string>() + expectTypeOf(foo).not.toBeAny() return foo }, }) + + // Optional: assert emits typing for hydrated event + h(LazyComp, { onHydrated: () => {} }) + // @ts-expect-error: hydrated does not receive a payload + h(LazyComp, { onHydrated: (e: unknown) => {} })
📜 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/nuxt/src/components/templates.ts(2 hunks)test/fixtures/basic-types/app/app-types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/src/components/templates.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/fixtures/basic-types/app/app-types.ts
🧠 Learnings (2)
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
test/fixtures/basic-types/app/app-types.ts
📚 Learning: 2024-11-05T20:04:39.622Z
Learnt from: Tofandel
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/runtime/client-delayed-component.ts:62-62
Timestamp: 2024-11-05T20:04:39.622Z
Learning: In Vue components, when typing the `hydrate` prop for hydration strategies like `hydrateOnInteraction`, use `type: null as PropType<Arguments<typeof hydrateOnInteraction>[0]>` to get the type directly from Vue's hydration methods. Also, remember that `HTMLElementEventMap` is not a type but an object; use `keyof HTMLElementEventMap` when defining prop types for events.
Applied to files:
test/fixtures/basic-types/app/app-types.ts
⏰ 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). (20)
- GitHub Check: test-fixtures (windows-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-benchmark
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: test-size
- GitHub Check: code
🔇 Additional comments (4)
test/fixtures/basic-types/app/app-types.ts (4)
15-15: Type-only import for public component types is correct.Importing
IslandComponentandLazyComponentviaimport typeavoids any runtime impact and matches the intent of this test.
378-378: No review needed for whitespace-only change.
386-386: No review needed for whitespace-only change.
433-433: No review needed for whitespace-only change.
| it('correctly includes event types with island components', () => { | ||
| const Comp = defineComponent({ | ||
| __typeProps: { | ||
| onClick: Function as PropType<(foo: string) => any>, | ||
| }, | ||
| }) | ||
| const IslandComp = Comp as unknown as IslandComponent<typeof Comp> | ||
| h(IslandComp, { | ||
| // @ts-expect-error: foo must be string, not number | ||
| onClick: (foo: number) => foo, | ||
| }) | ||
| h(IslandComp, { | ||
| onClick: (foo) => { | ||
| foo satisfies string | ||
| return foo | ||
| }, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Import PropType and harden the assertion to fail if the callback param is inferred as any.
PropTypeis used but not imported in this file; TS will complain unless it’s globally available (unlikely).foo satisfies stringwon’t fail iffooisany. Add explicitexpectTypeOfchecks to ensurefoois astringand notany.
Apply these changes:
- Add the missing type import (outside the shown hunk):
- import type { Ref, SlotsType } from 'vue'
+ import type { Ref, SlotsType, PropType } from 'vue'- Strengthen the assertion in this block:
h(IslandComp, {
onClick: (foo) => {
- foo satisfies string
+ // Ensure the parameter is strictly typed
+ expectTypeOf(foo).toEqualTypeOf<string>()
+ expectTypeOf(foo).not.toBeAny()
return foo
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('correctly includes event types with island components', () => { | |
| const Comp = defineComponent({ | |
| __typeProps: { | |
| onClick: Function as PropType<(foo: string) => any>, | |
| }, | |
| }) | |
| const IslandComp = Comp as unknown as IslandComponent<typeof Comp> | |
| h(IslandComp, { | |
| // @ts-expect-error: foo must be string, not number | |
| onClick: (foo: number) => foo, | |
| }) | |
| h(IslandComp, { | |
| onClick: (foo) => { | |
| foo satisfies string | |
| return foo | |
| }, | |
| }) | |
| }) | |
| // file: test/fixtures/basic-types/app/app-types.ts | |
| // ← existing imports | |
| import type { Ref, SlotsType, PropType } from 'vue' | |
| // …later, around line 387… | |
| it('correctly includes event types with island components', () => { | |
| const Comp = defineComponent({ | |
| __typeProps: { | |
| onClick: Function as PropType<(foo: string) => any>, | |
| }, | |
| }) | |
| const IslandComp = Comp as unknown as IslandComponent<typeof Comp> | |
| h(IslandComp, { | |
| // @ts-expect-error: foo must be string, not number | |
| onClick: (foo: number) => foo, | |
| }) | |
| h(IslandComp, { | |
| onClick: (foo) => { | |
| // Ensure the parameter is strictly typed | |
| expectTypeOf(foo).toEqualTypeOf<string>() | |
| expectTypeOf(foo).not.toBeAny() | |
| return foo | |
| }, | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In test/fixtures/basic-types/app/app-types.ts around lines 387 to 404, PropType
is used but not imported and the runtime-free "foo satisfies string" check won't
fail if foo is inferred as any; add a top-of-file import for PropType (e.g.
import { PropType } from 'vue') and replace the inline "foo satisfies string"
assertion with explicit expectTypeOf checks such as
expectTypeOf(foo).toEqualTypeOf<string>() and expectTypeOf(foo).not.toBeAny(),
ensuring expectTypeOf is imported from your test-assertions helper (e.g.
vitest/expect-type) if not already.
|
It looks fine! The essence of the problem is that the |
|
I wonder why the typecheck is failing then. I'll look at it again when I'm back at the computer. |
|
did you have any ideas why the type test fails? |
|
|
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
thank you for persisting! ❤️
2 participants
🔗 Linked issue
📚 Description
See playground or the code below. This is just a workaround.