refactor(nuxt,vite,webpack): allow builders to augment types #33427
Conversation
|
|
WalkthroughThe changes introduce resolveModulePath usage in Nuxt core to resolve the vite builder path and adjust nodeReferences based on the builder option. The schema package expands public exports, adds a new ViteOptions type, and updates ConfigSchema to use ViteOptions instead of a composite ViteConfig, also adding a viteEnvironmentApi boolean. The vite package removes a ViteBuildContext re-export and augments nuxt/schema by extending ViteOptions with optional $client, $server, and viteNode fields. Imports and types are realigned accordingly. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/vite/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (5)
packages/nuxt/src/core/nuxt.ts(3 hunks)packages/schema/src/index.ts(1 hunks)packages/schema/src/types/config.ts(1 hunks)packages/schema/src/types/schema.ts(3 hunks)packages/vite/src/index.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/schema/src/types/config.tspackages/vite/src/index.tspackages/schema/src/index.tspackages/nuxt/src/core/nuxt.tspackages/schema/src/types/schema.ts
🧠 Learnings (1)
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#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/core/nuxt.ts
🧬 Code graph analysis (3)
packages/schema/src/types/config.ts (1)
packages/schema/src/index.ts (2)
ViteOptions(4-4)ViteConfig(4-4)
packages/vite/src/index.ts (2)
packages/schema/src/index.ts (2)
ViteOptions(4-4)ViteConfig(4-4)packages/schema/src/types/config.ts (2)
ViteOptions(130-130)ViteConfig(96-127)
packages/schema/src/types/schema.ts (2)
packages/schema/src/index.ts (1)
ViteOptions(4-4)packages/schema/src/types/config.ts (1)
ViteOptions(130-130)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (7)
packages/nuxt/src/core/nuxt.ts (2)
25-25: LGTM!The choice of
exsolvefor module path resolution is appropriate for this use case.
255-257: Verify custom builder package existence before adding type reference.The custom builder string is pushed directly to
nodeReferenceswithout validation or path resolution, unlike the@nuxt/vite-builderhandling above. If the builder package doesn't exist or isn't properly installed, TypeScript will fail to resolve these types.Consider adding similar resolution logic for consistency and early error detection:
opts.nodeReferences.push({ types: relative(nuxt.options.buildDir, await resolveModulePath('@nuxt/vite-builder', { from: import.meta.url })) }) if (typeof nuxt.options.builder === 'string' && nuxt.options.builder !== '@nuxt/vite-builder') { - opts.nodeReferences.push({ types: nuxt.options.builder }) + try { + opts.nodeReferences.push({ types: relative(nuxt.options.buildDir, await resolveModulePath(nuxt.options.builder, { from: import.meta.url })) }) + } catch { + // Fallback to bare package name for TypeScript resolution + opts.nodeReferences.push({ types: nuxt.options.builder }) + } }Alternatively, verify that the current approach (bare package name) is intentional for custom builders and document this difference in behaviour.
packages/schema/src/types/config.ts (1)
129-131: ViteOptions hook looks goodNeat way to expose a builder-augmentable surface without touching the existing config contract. 👍
packages/schema/src/types/schema.ts (2)
1243-1247: Nice addition for the env API toggleThe dedicated flag keeps experimental Vite environment wiring explicit and matches the surrounding experimental options.
1660-1660: ViteOptions swap aligns with builder augmentationsUpdating
ConfigSchema['vite']to the newViteOptionssurface gives builders room to extend types without leaking those extras globally. 👍packages/vite/src/index.ts (1)
1-19: Augmentation looks on pointImport-only dependency keeps runtime clean, and the merged
$client,$server, andviteNodehooks bring the old shape back under the Vite builder umbrella.packages/schema/src/index.ts (1)
4-11: Great to see the extra exportsExporting
ViteOptions(and the additional module/router/debug types) gives external consumers parity with the internal surface.
| opts.nodeReferences.push({ path: resolve(nuxt.options.buildDir, 'types/runtime-config.d.ts') }) | ||
| opts.nodeReferences.push({ path: resolve(nuxt.options.buildDir, 'types/app.config.d.ts') }) | ||
| opts.nodeReferences.push({ types: 'nuxt' }) | ||
| opts.nodeReferences.push({ types: relative(nuxt.options.buildDir, resolveModulePath('@nuxt/vite-builder', { from: import.meta.url })) }) |
There was a problem hiding this comment.
Critical: Missing await for async resolveModulePath call.
resolveModulePath is an async function that returns a Promise<string>, but it's being called without await. This will pass a Promise object to relative() instead of the resolved path string, causing type generation to fail.
Apply this diff to fix the issue:
- opts.nodeReferences.push({ types: relative(nuxt.options.buildDir, resolveModulePath('@nuxt/vite-builder', { from: import.meta.url })) })
+ opts.nodeReferences.push({ types: relative(nuxt.options.buildDir, await resolveModulePath('@nuxt/vite-builder', { from: import.meta.url })) })📝 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.
| opts.nodeReferences.push({ types: relative(nuxt.options.buildDir, resolveModulePath('@nuxt/vite-builder', { from: import.meta.url })) }) | |
| opts.nodeReferences.push({ | |
| types: relative( | |
| nuxt.options.buildDir, | |
| await resolveModulePath('@nuxt/vite-builder', { from: import.meta.url }) | |
| ) | |
| }) |
🤖 Prompt for AI Agents
In packages/nuxt/src/core/nuxt.ts around line 254, the call to resolveModulePath
is async but is not awaited, so relative() receives a Promise; change the call
to await resolveModulePath(...) (i.e. pass the resolved string to relative) and
ensure the enclosing function is declared async (or otherwise awaiting this
operation) so the await is valid.
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33427 will not alter performanceComparing Summary
|
🔗 Linked issue
📚 Description
with #33262, the types for
viteare changing - and.I think it makes sense for$client,$serverandviteNodetypes to be defined by the builder rather than by the Nuxt schema more generallymight think of a way to deprecate the global types for these (shared through schema)