fix(nuxt): add auto-import declarations for shared/ context
#33978
Conversation
|
|
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
WalkthroughAdds generation of a new Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
this is a nice solution that avoids creating a separate unimport instance ❤️🙏
CodSpeed Performance ReportMerging #33978 will degrade performance by 15.78%Comparing Summary
Benchmarks breakdown
|
![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 (1)
packages/nuxt/src/imports/module.ts (1)
281-282: Consider usingaddTypeTemplatefor consistency with other type declaration files.This template generates a
.d.tsfile but usesaddTemplatewhereas the other type declaration files in this module (lines 260, 268) useaddTypeTemplate. UsingaddTypeTemplateensures proper handling of type-specific behaviour and registration.🔎 Proposed fix
- addTemplate({ + addTypeTemplate({ filename: 'types/shared-imports.d.ts',
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/imports/module.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/imports/module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/nuxt/src/imports/module.ts
🧠 Learnings (3)
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
Repo: nuxt/nuxt PR: 26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/nuxt/src/imports/module.ts
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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:
packages/nuxt/src/imports/module.ts
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/*.{ts,tsx,js,jsx,vue} : Add comments only to explain complex logic or non-obvious implementations
Applied to files:
packages/nuxt/src/imports/module.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 (6)
packages/nuxt/src/imports/module.ts (6)
5-5: LGTM!Import of
toTypeDeclarationFileis correctly added and used for generating the shared imports type declarations.
117-121: LGTM!The Nitro context capture via
nitro:inithook is correctly implemented. Theundefinedtype annotation properly reflects the async nature, and the fallback at line 194 handles the case when the context isn't yet available.
188-197: LGTM!The restructured call cleanly separates the Nuxt and Nitro contexts, exposing only the required methods for each. The fallback to an empty array when
nitroCtxis undefined is a sensible default.
265-266: LGTM!Good refactoring to extract repeated comment strings into constants, improving maintainability.
293-306: LGTM!The filtering logic correctly builds the intersection of Nuxt and Nitro imports. The approach of including both imports when paths differ to create union types is a sound solution for isomorphic functions.
217-217: LGTM!The function signature correctly uses
Pick<Unimport, 'getImports'>to expose only the required interface for the Nitro context, following the principle of minimal interface exposure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nuxt/src/imports/module.ts (1)
253-254: Consider more descriptive constant names.The constant names are clear, but could be slightly more specific to indicate they're template-related, such as
TEMPLATE_GENERATED_COMMENTandTEMPLATE_AUTO_IMPORTS_DISABLED_COMMENT. This is a minor naming preference.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/imports/module.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/imports/module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/nuxt/src/imports/module.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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.
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
Repo: nuxt/nuxt PR: 26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/nuxt/src/imports/module.ts
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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:
packages/nuxt/src/imports/module.ts
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/*.{ts,tsx,js,jsx,vue} : Add comments only to explain complex logic or non-obvious implementations
Applied to files:
packages/nuxt/src/imports/module.ts
🧬 Code graph analysis (1)
packages/nuxt/src/imports/module.ts (2)
packages/kit/src/index.ts (3)
addTypeTemplate(40-40)addTemplate(40-40)useNitro(39-39)packages/kit/src/template.ts (2)
addTypeTemplate(72-118)addTemplate(24-50)
⏰ 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 (2)
packages/nuxt/src/imports/module.ts (2)
2-2: LGTM! New imports support the shared-imports feature.The addition of
useNitroandtoTypeDeclarationFileappropriately supports the new shared imports functionality.Also applies to: 5-5
256-267: LGTM! Good refactoring to use constants.Using the defined constants improves maintainability and consistency across templates.
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
@cernymatej let's see if we can already merge this, preferring the ones from app/ context, and then refactor to a union (for more accurate types) when the unimport PR is merged.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nuxt/src/imports/module.ts (1)
269-302: UseaddTypeTemplateinstead ofaddTemplatefor proper type registration.This template generates a
.d.tsfile but usesaddTemplateinstead ofaddTypeTemplate. Based on the relevant code snippets frompackages/kit/src/template.ts, usingaddTypeTemplatewith{ shared: true }would:
- Automatically register the template in
sharedReferencesvia theprepare:typeshook- Expose global types to the Vue compiler via
vite.vue.script.globalTypeFiles🔎 Proposed fix
- addTemplate({ + addTypeTemplate({ filename: 'types/shared-imports.d.ts', getContents: async () => { // ... existing implementation }, - }) + }, { shared: true })
🧹 Nitpick comments (1)
packages/nuxt/src/imports/module.ts (1)
279-280: Consider adding debug logging when Nitro unimport is unavailable.When
nitro.unimportis undefined, the code silently falls back to an empty array. Whilst this is a reasonable graceful degradation, adding a debug log could aid troubleshooting.🔎 Optional enhancement
- const nitroImports = await nitro.unimport?.getImports() ?? [] + const nitroImports = await nitro.unimport?.getImports() ?? [] + if (!nitro.unimport) { + logger.debug('[nuxt:imports] Nitro unimport context not available, shared imports will be empty') + }Note: This would require importing
loggerif not already available in scope (it's imported from../utils.tson line 9).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/imports/module.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/imports/module.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/nuxt/src/imports/module.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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.
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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:
packages/nuxt/src/imports/module.ts
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/*.{ts,tsx,js,jsx,vue} : Add comments only to explain complex logic or non-obvious implementations
Applied to files:
packages/nuxt/src/imports/module.ts
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/*.{ts,tsx,vue} : Follow standard TypeScript conventions and best practices
Applied to files:
packages/nuxt/src/imports/module.ts
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/*.{ts,tsx,js,jsx,vue} : Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Applied to files:
packages/nuxt/src/imports/module.ts
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/*.{ts,tsx,js,jsx,vue} : Remove code that is not used or needed
Applied to files:
packages/nuxt/src/imports/module.ts
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
Repo: nuxt/nuxt PR: 26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/nuxt/src/imports/module.ts
🧬 Code graph analysis (1)
packages/nuxt/src/imports/module.ts (2)
packages/kit/src/index.ts (3)
addTypeTemplate(40-40)addTemplate(40-40)useNitro(39-39)packages/kit/src/template.ts (2)
addTypeTemplate(72-118)addTemplate(24-50)
⏰ 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, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, webpack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: test-size
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: test-benchmark
- GitHub Check: code
🔇 Additional comments (3)
packages/nuxt/src/imports/module.ts (3)
2-5: LGTM!The new imports are correctly added and necessary for the shared imports functionality:
useNitroto access Nitro's unimport context andtoTypeDeclarationFileto generate the type declaration file.
253-255: LGTM!Good practice extracting these header comments as constants for consistent reuse across both type declaration templates.
256-267: LGTM!The template correctly uses the new constants for consistent header generation whilst preserving the existing conditional logic for auto-import.
3 participants
🔗 Linked issue
fix #32714
fix #32646
closes #32671
📚 Description
This PR fixes a problem where the
shared/context didn't reference auto-import global type declarations.Initially, I intended to update #32671, but I decided against it since this implementation isn't based on that PR. I thought opening a new one would be clearer.
Here, we don't create a new unimport context for
shared/but instead create an intersection ofapp/andserver/auto-imports. Only the ones that are available in both contexts are put intotypes/shared-imports.d.ts. This way, it handles imports added byaddImports,addServerImportsand so on...@danielroe One thing I'm not entirely sure how to handle best is isomorphic functions like
useRuntimeConfig. Initially, I implemented it to always prefer the import path from app, but later I thought it might be better to create a union for these types instead. This would, however, require a change inunimport(see: unjs/unimport#489). What do you think?Important
Just to be clear - this should not be merged in the current state before unjs/unimport#489. Or instead we can use the previous implementation, which always uses the path from the nuxt app context.