fix(kit,schema): add moduleDependencies -> installModule
#33689
Conversation
|
|
WalkthroughAdds a Nuxt instance-level Map 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 |
![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 (2)
packages/schema/src/types/nuxt.ts (1)
98-102: Consider using the more specific ModuleToInstall type.The type uses bare
NuxtModulewithout type parameters, whereas the implementation inpackages/kit/src/module/install.ts(line 21) definesModuleToInstall = string | NuxtModule<ModuleOptions, Partial<ModuleOptions>, false>. This creates a minor type inconsistency where the schema type is less specific than the actual usage.Consider importing and using
ModuleToInstallfrom the kit module, or defining a matching type here for better type safety:+import type { ModuleOptions } from './module' + /** * Module options functions collected from moduleDependencies. * @internal */ -_moduleOptionsFunctions?: Map<string | NuxtModule, Array<() => { defaults?: Record<string, unknown>, overrides?: Record<string, unknown> }>> +_moduleOptionsFunctions?: Map<string | NuxtModule<ModuleOptions, Partial<ModuleOptions>, false>, Array<() => { defaults?: Record<string, unknown>, overrides?: Record<string, unknown> }>>packages/kit/src/module/install.ts (1)
173-196: Core fix implemented correctly, but consider extracting the duplication.This addition successfully achieves the PR objective by ensuring that legacy modules using the deprecated
installModulefunction receive merged options frommoduleDependencies. The logic correctly:
- Checks for the existence of
_moduleOptionsFunctionsbefore use- Merges options with correct precedence
- Passes merged options to lifecycle hooks and module execution
However, the merging logic (lines 178-192) is substantially duplicated from
installModules(lines 130-143). Consider extracting this into a shared helper function to reduce duplication and improve maintainability.Example refactoring:
function mergeModuleOptions( moduleToInstall: ModuleToInstall, configKey: keyof NuxtOptions | undefined, inlineOptions: any, nuxt: Nuxt ): any { if (!configKey || !nuxt._moduleOptionsFunctions) { return inlineOptions } const optionsFns = [ ...nuxt._moduleOptionsFunctions.get(moduleToInstall) || [], ...nuxt._moduleOptionsFunctions.get(configKey) || [], ] if (optionsFns.length === 0) { return inlineOptions } const overrides = [] as unknown as [Record<string, unknown> | undefined, ...Array<Record<string, unknown> | undefined>] const defaults: Array<Record<string, unknown> | undefined> = [] for (const fn of optionsFns) { const options = fn() overrides.push(options.overrides) defaults.push(options.defaults) } const merged = defu(inlineOptions, ...overrides, nuxt.options[configKey], ...defaults) ;(nuxt.options[configKey] as any) = merged return merged }Then use it in both locations:
// In installModules: const mergedOptions = mergeModuleOptions(moduleToInstall, configKey, inlineOptions, nuxt) await callLifecycleHooks(nuxtModule, meta, mergedOptions, nuxt) await callModule(nuxtModule, meta, mergedOptions, ...) // In installModule: const mergedOptions = mergeModuleOptions(moduleToInstall, configKey, inlineOptions, nuxt) await callLifecycleHooks(nuxtModule, meta, mergedOptions, nuxt) await callModule(nuxtModule, meta, mergedOptions, ...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kit/src/module/install.ts(5 hunks)packages/kit/test/module.test.ts(2 hunks)packages/schema/src/types/nuxt.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/schema/src/types/nuxt.tspackages/kit/test/module.test.tspackages/kit/src/module/install.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/kit/test/module.test.ts
🧠 Learnings (2)
📚 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/schema/src/types/nuxt.ts
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write unit tests for core functionality using `vitest`
Applied to files:
packages/kit/test/module.test.ts
🧬 Code graph analysis (3)
packages/schema/src/types/nuxt.ts (1)
packages/schema/src/index.ts (1)
NuxtModule(10-10)
packages/kit/test/module.test.ts (3)
packages/nuxt/src/core/nuxt.ts (1)
loadNuxt(753-871)packages/kit/src/module/define.ts (1)
defineNuxtModule(23-35)packages/kit/src/module/install.ts (1)
installModule(158-197)
packages/kit/src/module/install.ts (1)
packages/schema/src/types/config.ts (1)
NuxtOptions(82-94)
⏰ 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: code
- GitHub Check: build
🔇 Additional comments (7)
packages/kit/test/module.test.ts (3)
104-112: LGTM! Clean approach to module factory with metadata.The use of
Object.assignto attachgetMetaas a static method to the module function is a clean pattern that satisfies the module interface whilst allowing test inspection of the loaded options viaglobalThis.someModuleOptions.
118-118: LGTM! Proper test cleanup.The addition of cleanup for
globalThis.someModuleOptionsinbeforeEachensures proper test isolation.
305-370: Excellent test coverage for the PR objective.This test comprehensively validates that
moduleDependenciesconfiguration is properly applied wheninstallModuleis called explicitly. It correctly verifies:
- Module execution order
- Single module instantiation (no duplicate loading)
- Complete option merging precedence: inline options > module overrides > user configuration > module defaults
The test assertions at lines 358-369 thoroughly cover all the merging scenarios.
packages/kit/src/module/install.ts (4)
44-44: LGTM! Safe initialisation of shared storage.The use of the nullish coalescing assignment operator (
||=) ensures the Map is only created if it doesn't already exist, providing safe initialisation of the shared module options storage.
84-88: LGTM! Correct accumulation of module dependency options.The code correctly accumulates multiple
moduleDependenciesconfigurations by preserving existing entries before adding new option functions. The lazy evaluation pattern (storing functions rather than values) is appropriate.
126-148: LGTM! Correct option merging for module dependencies.The option merging logic correctly implements the precedence: inline options (highest) → module overrides → user configuration → module defaults (lowest). The use of
defuwith the spread operators properly applies this precedence, and updatingnuxt.options[configKey]ensures downstream code sees the merged configuration.Note: This merging logic is duplicated in the
installModulefunction (lines 173-196). See separate comment about potential refactoring.
150-151: LGTM! Proper cleanup of temporary storage.Cleaning up
_moduleOptionsFunctionsafter all modules are processed is good practice. The deprecatedinstallModulefunction correctly handles the case where this storage may not exist (line 177 check).
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33689 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kit/src/module/install.ts(5 hunks)
🧰 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/kit/src/module/install.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/kit/src/module/install.ts
🧬 Code graph analysis (1)
packages/kit/src/module/install.ts (2)
packages/schema/src/index.ts (1)
NuxtOptions(4-4)packages/schema/src/types/config.ts (1)
NuxtOptions(82-94)
⏰ 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). (3)
- GitHub Check: code
- GitHub Check: codeql (actions)
- GitHub Check: build
🔇 Additional comments (4)
packages/kit/src/module/install.ts (4)
44-44: LGTM!The initialisation of the shared
_moduleOptionsFunctionsMap is correctly placed and uses the appropriate nullish coalescing assignment operator.
83-89: LGTM!The logic correctly accumulates module option merge functions, preserving existing entries and appending new callbacks.
152-153: LGTM!The cleanup of the temporary Map after module installation is complete prevents memory leaks and is correctly placed after all modules have been processed.
192-192: Verify the merge order includesinlineOptionscorrectly.The merge order here is
defu(inlineOptions, ...overrides, nuxt.options[configKey], ...defaults), which givesinlineOptionsthe highest priority. However, ininstallModules(line 144), the merge isdefu(...overrides, nuxt.options[configKey], ...defaults)without includinginlineOptionsin the defu call.This difference may be intentional (since
installModuleis the legacy API and might need different semantics), but please confirm this is the desired behaviour. IfinlineOptionsshould have highest priority in both paths, the fix suggested for lines 148-149 should address this.
| await callLifecycleHooks(nuxtModule, meta, inlineOptions, nuxt) | ||
| await callModule(nuxtModule, meta, inlineOptions, resolvedModulePath, moduleToInstall, localLayerModuleDirs, buildTimeModuleMeta, nuxt) |
There was a problem hiding this comment.
Critical: Inconsistent options handling between installModules and installModule.
These calls pass inlineOptions to lifecycle hooks and module execution, but the merged options (from lines 129-146) are only applied to nuxt.options[configKey] and not propagated here. In contrast, installModule (lines 197-198) passes mergedOptions to the same functions. This inconsistency means:
- In
installModules: modules receiveinlineOptionsas the parameter but must read merged options fromnuxt.options[configKey] - In
installModule: modules receivemergedOptionsas the parameter
If modules rely on the options parameter (rather than reading from config), they won't see the merged dependency options when installed via installModules.
Apply this diff to pass merged options consistently:
+ let mergedOptions = inlineOptions
if (optionsFns.length > 0) {
const overrides = [] as unknown as [Record<string, unknown> | undefined, ...Array<Record<string, unknown> | undefined>]
const defaults: Array<Record<string, unknown> | undefined> = []
for (const fn of optionsFns) {
const options = fn()
overrides.push(options.overrides)
defaults.push(options.defaults)
}
if (configKey) {
+ mergedOptions = defu(inlineOptions, ...overrides, nuxt.options[configKey], ...defaults)
- ;(nuxt.options[configKey] as any) = defu(...overrides, nuxt.options[configKey], ...defaults)
+ ;(nuxt.options[configKey] as any) = mergedOptions
}
}
- await callLifecycleHooks(nuxtModule, meta, inlineOptions, nuxt)
- await callModule(nuxtModule, meta, inlineOptions, resolvedModulePath, moduleToInstall, localLayerModuleDirs, buildTimeModuleMeta, nuxt)
+ await callLifecycleHooks(nuxtModule, meta, mergedOptions, nuxt)
+ await callModule(nuxtModule, meta, mergedOptions, resolvedModulePath, moduleToInstall, localLayerModuleDirs, buildTimeModuleMeta, nuxt)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/kit/src/module/install.ts around lines 148-149, the code calls
callLifecycleHooks(...) and callModule(...) passing inlineOptions, but the
mergedOptions computed earlier (lines ~129-146) are not used; update these two
calls to pass mergedOptions (the fully merged dependency options) instead of
inlineOptions so module lifecycle hooks and module execution receive the same
merged options as installModule does, ensuring consistent behavior regardless of
installation path.
| const optionsFns = [ | ||
| ...nuxt._moduleOptionsFunctions.get(moduleToInstall) || [], | ||
| ...nuxt._moduleOptionsFunctions.get(configKey) || [], | ||
| ] |
There was a problem hiding this comment.
Missing meta.name check when collecting option functions.
The option function collection only checks moduleToInstall and configKey, but doesn't check for meta.name like installModules does on line 131. This means if a module dependency specifies options keyed by the module's name, those options won't be applied when using installModule.
Apply this diff to match the logic in installModules:
const optionsFns = [
...nuxt._moduleOptionsFunctions.get(moduleToInstall) || [],
+ ...meta?.name ? nuxt._moduleOptionsFunctions.get(meta.name) || [] : [],
...nuxt._moduleOptionsFunctions.get(configKey) || [],
]📝 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.
| const optionsFns = [ | |
| ...nuxt._moduleOptionsFunctions.get(moduleToInstall) || [], | |
| ...nuxt._moduleOptionsFunctions.get(configKey) || [], | |
| ] | |
| const optionsFns = [ | |
| ...nuxt._moduleOptionsFunctions.get(moduleToInstall) || [], | |
| ...meta?.name ? nuxt._moduleOptionsFunctions.get(meta.name) || [] : [], | |
| ...nuxt._moduleOptionsFunctions.get(configKey) || [], | |
| ] |
🤖 Prompt for AI Agents
In packages/kit/src/module/install.ts around lines 180 to 183, the code
collecting option functions misses checking the module's meta.name (as
installModules does on line 131), so options keyed by the module's declared name
are not applied; update the optionsFns assembly to also include
nuxt._moduleOptionsFunctions.get(meta?.name) (guarding meta may be undefined) so
it merges functions from moduleToInstall, configKey, and the module's meta.name
before proceeding.
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
🔗 Linked issue
📚 Description
this ensures that legacy modules which are still using
installModulehave access to any changes to options that occur through registeringmoduleDependencies