feat(kit,nuxt,schema): allow modules to specify dependencies #33063
Conversation
|
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
packages/nuxt/src/core/nuxt.ts
Outdated
| // for (const [key, options] of modules) { | ||
| // await installModule(key, options) | ||
| // } | ||
|
|
There was a problem hiding this comment.
| // for (const [key, options] of modules) { | |
| // await installModule(key, options) | |
| // } |
packages/schema/src/types/module.ts
Outdated
| // TODO: review option name | ||
| // TODO: type constraints for module options | ||
| modules?: Record<string, ModuleDependencyMeta> |
There was a problem hiding this comment.
let's review the name, as I'm not sure modules is good enough
we might also be able to do something with modules.d.ts to ensure the types match ....
CodSpeed Performance ReportMerging #33063 will not alter performanceComparing Summary
Footnotes |
| optional?: boolean | ||
| } |
There was a problem hiding this comment.
| optional?: boolean | |
| } | |
| optional?: boolean | |
| strict?: boolean | |
| } |
How about adding an additional field here to enforce module version?
Sometimes it's not ideal to just print out warning message, but preferably bail out of the whole build process.
There was a problem hiding this comment.
in honesty I'm not sure about including version validation at all as we already have hasNuxtModuleCompatibility
what do you think @harlan-zw?
There was a problem hiding this comment.
i'd be for less complexity to start and if peerDependencies can handle it great
in some instances we have a hard dependency, though, not sure how this is handled 🤔 i guess we get end users to install module sub dependencies? it would make the resolutions easier
|
Warning Rate limit exceeded@danielroe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds multi-module installation and dependency-resolution features. packages/kit: exports new helpers ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt/src/core/nuxt.ts (1)
424-426: Plumb and pass resolved module paths to installModules (prevents duplicate dependency installs).Currently
installModulesreceivesmodulePaths(watch paths), but it expects a set of resolved module paths for deduping. ReturnresolvedModulePathsfromresolveModules, destructure it, and pass it in.@@ - const { paths: modulePaths, modules } = await resolveModules(nuxt) + const { paths: modulePaths, modules, resolvedModulePaths } = await resolveModules(nuxt) @@ - await installModules(modules, modulePaths, nuxt) + await installModules(modules, resolvedModulePaths, nuxt) @@ - return { - paths, - modules, - } + return { + paths, + modules, + resolvedModulePaths, + }Also applies to: 557-558, 946-950
♻️ Duplicate comments (1)
packages/schema/src/types/module.ts (1)
83-86: Renamemodulestodependenciesfor clarityThe property name is ambiguous;
dependenciesordependsOnbetter communicates intent and avoids confusion withnuxt.options.modules. This also echoes prior feedback on the naming.- // TODO: review option name - // TODO: type constraints for module options - modules?: Record<string, ModuleDependencyMeta> + // TODO: type constraints for module options + /** Module dependencies declared by this module */ + dependencies?: Record<string, ModuleDependencyMeta>If renaming is deferred, consider aliasing both names for one minor version with a deprecation notice to ease migration.
🧹 Nitpick comments (12)
packages/schema/src/types/module.ts (1)
66-71: Clarify semantics and allow configurable enforcement on version mismatchAdd concise JSDoc describing precedence of
overridesvsdefaultsand introduce an optionalenforceflag to switch from warn-only to error-on-mismatch. This keeps defaults/warns behaviour unchanged while enabling stricter consumers.export interface ModuleDependencyMeta { version?: string overrides?: Record<string, unknown> defaults?: Record<string, unknown> optional?: boolean + /** + * Behaviour when `version` does not satisfy the installed dependency. + * - 'warn' (default): log a warning + * - 'error': throw and fail the build + */ + enforce?: 'warn' | 'error' }packages/kit/src/index.ts (1)
3-3: MarkinstallModuleas deprecated at the re‑export siteThe aggregated re‑export hides deprecation. Split it to attach a JSDoc deprecation tag so downstream users see it in IDEs and types.
-export { getDirectory, installModule, installModules, loadNuxtModuleInstance, normalizeModuleTranspilePath, resolveModuleWithOptions } from './module/install' +/** @deprecated Use `installModules` instead. */ +export { installModule } from './module/install' +export { getDirectory, installModules, loadNuxtModuleInstance, normalizeModuleTranspilePath, resolveModuleWithOptions } from './module/install'packages/kit/test/module.test.ts (3)
112-114: Restore mocks between tests
vi.spyOnis used later without restoration. Reset to prevent leakage across tests.afterEach(async () => { await nuxt?.close() + vi.restoreAllMocks() })
94-107: Clean up the temporary workspace created for dependencies testsRemove the temp directory at the end to avoid polluting the repo workspace.
beforeAll(async () => { @@ }) + afterAll(async () => { + await rm(tempDir, { recursive: true, force: true }) + })Also applies to: 112-114
11-11: Keep a compatibility test forinstallModule, but add a companion test forinstallModulesGiven the planned deprecation, add a small smoke test for the new bulk API to guard against regressions while retaining this legacy test.
I can add a new
describe('installModules', ...)block mirroring the hook assertions, ensuring it writesinstall/upgradeonce across a batch call. Want me to draft it?packages/nuxt/src/core/nuxt.ts (2)
904-913: Avoid adding sentinel strings ('function'/'string') to resolvedModulePaths.Only add when
resolvedPathexists; otherwise you end up inserting the literal wordsfunction/string.- const path = resolvedModule.resolvedPath || typeof resolvedModule.module - if (typeof path === 'string') { - resolvedModulePaths.add(path) - } + if (resolvedModule.resolvedPath) { + resolvedModulePaths.add(resolvedModule.resolvedPath) + }
935-944: Same issue as above: only add actual resolved paths.Replicate the fix here to avoid polluting the set.
- const path = resolvedModule.resolvedPath || typeof resolvedModule.module - if (typeof path === 'string') { - resolvedModulePaths.add(path) - } + if (resolvedModule.resolvedPath) { + resolvedModulePaths.add(resolvedModule.resolvedPath) + }packages/kit/src/module/install.ts (5)
29-33: Public API marked as internal.
installModulesis exported publicly from kit; drop the@internalmarker to avoid confusion in generated docs.-/** - * Installs a set of modules on a Nuxt instance. - * @internal - */ +/** + * Installs a set of modules on a Nuxt instance. + */
42-47: Filter undefined config keys to avoid set pollution.
Promise.allcan yieldundefinedvalues; filter before constructing theSet.- const inlineConfigKeys = new Set( - await Promise.all([...modulesToInstall].map(([mod]) => typeof mod !== 'string' && mod.getMeta?.()?.then(r => r.configKey))), - ) + const inlineConfigKeys = new Set( + (await Promise.all( + [...modulesToInstall].map(([mod]) => typeof mod !== 'string' && mod.getMeta?.()?.then(r => r.configKey)), + )).filter(Boolean) as string[], + )
64-69: Avoid non-null assertion on resolvedModulePath when reading package.json.If the current module was provided inline (function),
res.resolvedModulePathisundefined. Pass a filteredfromlist instead.- const pkg = await readPackageJSON(name, { from: [res.resolvedModulePath!, ...nuxt.options.modulesDir] }).catch(() => null) + const from = [res.resolvedModulePath, ...nuxt.options.modulesDir].filter(Boolean) as string[] + const pkg = await readPackageJSON(name, { from }).catch(() => null)
106-124: Make overrides/defaults merge robust and clearer.Avoid passing
undefinedentries intodefuand remove the tuple type hack.- 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) - } - ;(nuxt.options[configKey] as any) = defu(...overrides, nuxt.options[configKey], ...defaults) - } + if (optionsFns.length > 0) { + const overrideObjs = optionsFns.map(fn => fn().overrides).filter(Boolean) as Record<string, unknown>[] + const defaultObjs = optionsFns.map(fn => fn().defaults).filter(Boolean) as Record<string, unknown>[] + ;(nuxt.options[configKey] as any) = defu(...overrideObjs, nuxt.options[configKey], ...defaultObjs) + }
135-139: Clarify deprecation guidance.Point users to the new flow to smooth migration.
-/** - * Installs a module on a Nuxt instance. - * @deprecated Use module dependencies. - */ +/** + * Installs a module on a Nuxt instance. + * @deprecated Prefer `installModules` and module dependency metadata (`getDependencyMeta`). + */
📜 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 (7)
packages/kit/src/index.ts(1 hunks)packages/kit/src/module/define.ts(3 hunks)packages/kit/src/module/install.ts(4 hunks)packages/kit/test/module.test.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(4 hunks)packages/schema/src/index.ts(1 hunks)packages/schema/src/types/module.ts(3 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/index.tspackages/kit/src/index.tspackages/kit/test/module.test.tspackages/kit/src/module/define.tspackages/schema/src/types/module.tspackages/nuxt/src/core/nuxt.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 (3)
📚 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/kit/src/index.tspackages/nuxt/src/core/nuxt.tspackages/kit/src/module/install.ts
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#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
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/e2e/**/*.{ts,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
Applied to files:
packages/kit/test/module.test.ts
🧬 Code graph analysis (4)
packages/kit/test/module.test.ts (3)
packages/kit/src/index.ts (3)
loadNuxt(10-10)defineNuxtModule(2-2)logger(32-32)packages/nuxt/src/core/nuxt.ts (1)
loadNuxt(735-846)packages/kit/src/module/define.ts (1)
defineNuxtModule(23-35)
packages/kit/src/module/define.ts (2)
packages/schema/src/index.ts (1)
ModuleDependencyMeta(8-8)packages/schema/src/types/module.ts (1)
ModuleDependencyMeta(66-71)
packages/nuxt/src/core/nuxt.ts (1)
packages/kit/src/module/install.ts (2)
installModules(33-129)resolveModuleWithOptions(155-185)
packages/kit/src/module/install.ts (6)
packages/schema/src/types/module.ts (3)
NuxtModule(97-127)ModuleOptions(34-34)ModuleMeta(6-31)packages/kit/src/index.ts (10)
installModules(3-3)useNuxt(21-21)loadNuxtModuleInstance(3-3)resolveModuleWithOptions(3-3)logger(32-32)installModule(3-3)resolveAlias(28-28)directoryToURL(35-35)getDirectory(3-3)normalizeModuleTranspilePath(3-3)packages/schema/src/types/nuxt.ts (1)
Nuxt(89-116)packages/schema/src/types/config.ts (2)
NuxtOptions(81-93)NuxtConfig(49-66)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)
🔇 Additional comments (4)
packages/schema/src/types/module.ts (1)
117-117: Public API surface addition looks goodAdding
getDependencyMetatoNuxtModuleis a sensible, minimal surface for consumers that need dependency metadata.packages/kit/src/module/define.ts (1)
4-4: Type import changes are consistent with schema exposureImporting
ModuleDependencyMetahere aligns with the new public types in@nuxt/schema.packages/schema/src/index.ts (1)
8-8: Type export addition is correctRe‑exporting
ModuleDependencyMetafrom the schema index keeps the API cohesive.packages/nuxt/src/core/nuxt.ts (1)
9-9: Import changes align with new kit APIs.The shift to
installModulesandresolveModuleWithOptionsfrom@nuxt/kitlooks correct and consistent with the PR’s intent.
packages/kit/src/module/define.ts
Outdated
| normalizedModule.getDependencyMeta = getDependencyMeta | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Attach getDependencyMeta to the normalised module
Good placement; once the return type is corrected, this keeps the public API minimal and discoverable.
Run to confirm types are now consistent:
🏁 Script executed:
#!/bin/bash
# Verify declaration vs implementation signatures
rg -nP -C1 "getDependencyMeta\\?:\\s*\\(\\)\\s*=>\\s*Record<string,\\s*ModuleDependencyMeta>\\s*\\|\\s*undefined" packages/schema/src/types/module.ts
rg -nP -C1 "(?s)function\\s+getDependencyMeta\\s*\\(\\)\\s*:\\s*Record<string,\\s*ModuleDependencyMeta>\\s*\\|\\s*undefined" packages/kit/src/module/define.tsLength of output: 423
🏁 Script executed:
#!/bin/bash
# Locate all occurrences of getDependencyMeta in implementation file
rg -n "getDependencyMeta" -C3 packages/kit/src/module/define.ts
# Specifically find the function definition signature
rg -nP "function\s+getDependencyMeta\s*\(.*\)" -C3 packages/kit/src/module/define.tsLength of output: 805
Align getDependencyMeta signature with its type declaration
The implementation in packages/kit/src/module/define.ts (lines 78–80) is currently declared as:
function getDependencyMeta(): ModuleDependencyMeta | undefined {
return module.modules
}—but the schema type in packages/schema/src/types/module.ts (line 117) specifies:
getDependencyMeta?: () => Record<string, ModuleDependencyMeta> | undefined• Update the implementation signature in define.ts to:
- function getDependencyMeta(): ModuleDependencyMeta | undefined {
+ function getDependencyMeta(): Record<string, ModuleDependencyMeta> | undefined {
return module.modules
}• Ensure that module.modules is indeed a Record<string, ModuleDependencyMeta> (or undefined) and handle missing entries as needed.
• Retain the assignment:
normalizedModule.getDependencyMeta = getDependencyMetaThis change will keep the public API minimal and discoverable while restoring type consistency across the codebase.
🤖 Prompt for AI Agents
In packages/kit/src/module/define.ts around lines 148–149, change the
getDependencyMeta implementation signature to match the schema: make it return
Record<string, ModuleDependencyMeta> | undefined (instead of a single
ModuleDependencyMeta | undefined), ensure it returns module.modules (or
undefined) and defensively handle the case where module.modules is
missing/invalid (e.g., return undefined or an empty object as appropriate), and
keep the existing assignment normalizedModule.getDependencyMeta =
getDependencyMeta so the public API and types align with
packages/schema/src/types/module.ts.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt/src/core/nuxt.ts (1)
900-907: Fix erroneous path computation forresolvedModule.moduleThe current logic
const path = resolvedModule.resolvedPath || typeof resolvedModule.module if (typeof path === 'string') { resolvedModulePaths.add(path) }will push the literal
"string"or"function"into yourSet, breaking the “already seen” guard. Instead, only use the module identifier when it’s actually a string path.Please apply the following patch at all three occurrences:
- packages/kit/src/module/install.ts (around line 89)
- packages/nuxt/src/core/nuxt.ts (around lines 903–904)
- packages/nuxt/src/core/nuxt.ts (around lines 934–935)
- const path = resolvedModule.resolvedPath || typeof resolvedModule.module - if (typeof path === 'string') { - resolvedModulePaths.add(path) - } + const p = resolvedModule.resolvedPath + || (typeof resolvedModule.module === 'string' ? resolvedModule.module : undefined) + if (p) { + resolvedModulePaths.add(p) + }This ensures only valid string paths are recorded.
♻️ Duplicate comments (2)
packages/schema/src/types/module.ts (1)
116-116: Return type is fine; consider naming parity
getModuleDependenciesname is clear. If you rename the definition key todependencies(as discussed in the PR), mirror it here for consistency.packages/kit/src/module/install.ts (1)
250-264: Make transpile-path normalisation OS-agnosticSplitting on
'node_modules/'breaks on Windows.-export const normalizeModuleTranspilePath = (p: string) => { - return getDirectory(p).split('node_modules/').pop() as string -} +export const normalizeModuleTranspilePath = (p: string) => { + return (getDirectory(p).split(/[/\\]node_modules[/\\]/).pop() || getDirectory(p)) as string +}
🧹 Nitpick comments (6)
packages/schema/src/types/module.ts (1)
83-84: Tighten typing and doc formoduleDependencies
- Consider allowing
Awaitable<Record<...>>for the function form to match other APIs that accept async resolution.- Briefly document precedence semantics of
overridesvsdefaults(overrides > user config > defaults).packages/kit/src/module/install.ts (5)
106-124: Option merge precedence is good; add brief comment
defu(...overrides, nuxt.options[configKey], ...defaults)gives overrides > user > defaults. A one-line comment would prevent regressions.- ;(nuxt.options[configKey] as any) = defu(...overrides, nuxt.options[configKey], ...defaults) + // precedence: dependency overrides > user config > dependency defaults + ;(nuxt.options[configKey] as any) = defu(...overrides, nuxt.options[configKey], ...defaults)
131-153: Deprecation message could be clearerThe JSDoc deprecates
installModulebut the message says “Use module dependencies.” Consider pointing toinstallModulesand dependency metadata.- * @deprecated Use module dependencies. + * @deprecated Prefer `installModules(...)` and module `moduleDependencies` metadata.
297-331: Transpile and modulesDir augmentation: edge cases
parseNodeModulePath(modulePath)may return emptydirfor non-node_modules; fallback looks good. Add a guard to avoid pushing duplicates intobuild.transpile.- When pushing
join(moduleRoot, 'node_modules')intomodulesDir, ensure it exists to avoid bloating the search list.- nuxt.options.build.transpile.push(normalizeModuleTranspilePath(moduleRoot)) + const transpileKey = normalizeModuleTranspilePath(moduleRoot) + if (!nuxt.options.build.transpile.includes(transpileKey)) { + nuxt.options.build.transpile.push(transpileKey) + } @@ - if (moduleRoot !== moduleToInstall && !localLayerModuleDirs.some(dir => directory.startsWith(dir))) { - nuxt.options.modulesDir.push(join(moduleRoot, 'node_modules')) - } + if (moduleRoot !== moduleToInstall && !localLayerModuleDirs.some(dir => directory.startsWith(dir))) { + const nm = join(moduleRoot, 'node_modules') + if (!nuxt.options.modulesDir.includes(nm) && existsSync(nm)) { + nuxt.options.modulesDir.push(nm) + } + }
50-63: No-op fast-path is asymmetricEarly-continue only when
optionalis truthy and no other fields exist. If the intent is “specifying onlyversionshould warn but not auto-install”, add that case explicitly.
172-178: Alias/path resolution suffixesThe suffix list looks comprehensive. Consider documenting that order affects which entry is preferred when multiple exist.
📜 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 (5)
packages/kit/src/module/define.ts(2 hunks)packages/kit/src/module/install.ts(4 hunks)packages/kit/test/module.test.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(4 hunks)packages/schema/src/types/module.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kit/test/module.test.ts
- packages/kit/src/module/define.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:
packages/schema/src/types/module.tspackages/kit/src/module/install.tspackages/nuxt/src/core/nuxt.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/kit/src/module/install.tspackages/nuxt/src/core/nuxt.ts
🧬 Code graph analysis (2)
packages/kit/src/module/install.ts (4)
packages/schema/src/types/module.ts (3)
NuxtModule(96-126)ModuleOptions(34-34)ModuleMeta(6-31)packages/schema/src/types/config.ts (2)
NuxtOptions(81-93)NuxtConfig(49-66)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)
packages/nuxt/src/core/nuxt.ts (1)
packages/kit/src/module/install.ts (2)
installModules(33-129)resolveModuleWithOptions(155-185)
🔇 Additional comments (2)
packages/nuxt/src/core/nuxt.ts (1)
9-9: Import changes look goodSwitch to kit’s
installModules/resolveModuleWithOptionsis appropriate.packages/kit/src/module/install.ts (1)
29-33: Overall: bulk install entrypoint is solidAPI shape and internal plumbing look good and align with Nuxt core changes.
|
Looking good to me ❤️ Shall we also document the updated definition in https://nuxt.com/docs/4.x/guide/going-further/modules#module-definition as well? |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/2.guide/3.going-further/3.modules.md (1)
500-529: Update “Using Other Modules” to reflectinstallModuledeprecation.Show
moduleDependenciesas the preferred pattern; keepinstallModulebehind a deprecation note with migration guidance.-#### Using Other Modules in Your Module - -If your module depends on other modules, you can add them by using Nuxt Kit's `installModule` utility. For example, if you wanted to use Nuxt Tailwind in your module, you could add it as below: +#### Using Other Modules in Your Module + +Prefer declaring dependencies via `moduleDependencies` so Nuxt can apply option overrides/defaults before modules run. `installModule` is deprecated. + +Example: + +```ts +import { defineNuxtModule } from '@nuxt/kit' + +export default defineNuxtModule({ + moduleDependencies: { + '@nuxtjs/tailwindcss': { + defaults: { + exposeConfig: true, + config: { darkMode: 'class' } + } + } + } +}) +``` + +::warning +`installModule` is deprecated. Use `moduleDependencies` for configuration-time coordination between modules. +::
♻️ Duplicate comments (5)
packages/kit/src/module/install.ts (5)
45-47: Avoid calling.thenon possibly undefined; use an async mapper.
mod.getMeta?.()?.then(...)can evaluateundefined.then(...). Map to an async function and await safely.- const inlineConfigKeys = new Set( - await Promise.all([...modulesToInstall].map(([mod]) => typeof mod !== 'string' && mod.getMeta?.()?.then(r => r.configKey))), - ) + const inlineConfigKeys = new Set( + await Promise.all( + [...modulesToInstall].map(async ([mod]) => + typeof mod !== 'string' ? (await mod.getMeta?.())?.configKey : undefined, + ), + ), + )
73-79: Guardfrompaths forreadPackageJSON.
res.resolvedModulePath!may be undefined; avoid passing undefined in thefromarray.- const pkg = await readPackageJSON(name, { from: [res.resolvedModulePath!, ...nuxt.options.modulesDir] }).catch(() => null) + const from = [res.resolvedModulePath, ...nuxt.options.modulesDir].filter(Boolean) as string[] + const pkg = await readPackageJSON(name, { from }).catch(() => null)
89-91: Inverted optional-dependency check (auto-install).As written, required deps (
optional === false) are skipped. Skip only when the dep is optional.- if (value.optional === false) { + if (value.optional === true) { continue }
100-103: Do not add literal"string"/"function"intoresolvedModulePaths.The fallback to
typeof resolvedModule.moduleis incorrect and pollutes the set.- const path = resolvedModule.resolvedPath || typeof resolvedModule.module - if (typeof path === 'string') { - resolvedModulePaths.add(path) - } + const p = resolvedModule.resolvedPath || (typeof resolvedModule.module === 'string' ? resolvedModule.module : undefined) + if (p) { + resolvedModulePaths.add(p) + }
276-278: Normalise transpile path cross‑platform.Split on both
/and\aroundnode_modules.export const normalizeModuleTranspilePath = (p: string) => { - return getDirectory(p).split('node_modules/').pop() as string + return getDirectory(p).split(/[/\\]node_modules[/\\]/).pop() as string }
🧹 Nitpick comments (6)
packages/kit/src/module/install.ts (2)
67-71: Resolution check never triggers.
resolveModuleWithOptionsalways returns an object with a truthymodule. If intent was “error when not found”, checkresolvedPathinstead (and the dependency’s optionality).- if (!resolvedModule?.module) { + if (!resolvedModule?.resolvedPath && value.optional !== true) { const message = `Could not resolve \`${name}\` (specified as a dependency of ${moduleToAttribute}).` error = new TypeError(message) continue }
121-139: Option merge order is non-obvious; codify precedence.Current
defu(...overrides, nuxt.options[configKey], ...defaults)gives precedence to the first override object only. If multiple dependencies provide overrides/defaults, document or enforce deterministic precedence (e.g. last-wins for overrides).Would you prefer “last override wins”? If yes, I can propose a small reducer to fold overrides/defaults in the desired order.
test/fixtures/basic-types/config-types.ts (1)
44-61: Add a function-form test formoduleDependencies.Also validate the callable form
(nuxt) => ModuleDependenciesand anoptional: truecase.Example:
defineNuxtModule({ moduleDependencies: (nuxt) => ({ '@nuxt/devtools': { optional: true, defaults: { enabled: true } }, }), })docs/2.guide/3.going-further/3.modules.md (1)
180-196: ClarifymoduleDependenciessemantics (optional, version behaviour).
- State explicitly: optional: true ⇒ not auto-installed; default/absent ⇒ auto-install.
- Ensure wording matches runtime (currently throws on version mismatch).
I can open a PR to align the phrasing and add a short table of behaviours.
packages/schema/src/types/module.ts (2)
66-71: Document fields ofModuleDependencyMeta.Add JSDoc to define precedence and behaviours (auto-install, version handling).
export interface ModuleDependencyMeta<T = Record<string, unknown>> { - version?: string - overrides?: Partial<T> - defaults?: Partial<T> - optional?: boolean + /** Semver range that the installed module version should satisfy. */ + version?: string + /** Keys that override user config for the target module (highest precedence). */ + overrides?: Partial<T> + /** Keys that apply if not provided by user config (higher than module defaults). */ + defaults?: Partial<T> + /** When true, Nuxt will not auto-install this dependency. */ + optional?: boolean }
87-87: Allow function form to returnundefined.Align with
getModuleDependencies?: () => ModuleDependencies | undefined.- moduleDependencies?: ModuleDependencies | ((nuxt: Nuxt) => ModuleDependencies) + moduleDependencies?: ModuleDependencies | ((nuxt: Nuxt) => ModuleDependencies | undefined)
📜 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 (7)
docs/2.guide/3.going-further/3.modules.md(1 hunks)packages/kit/src/module/install.ts(4 hunks)packages/kit/test/module.test.ts(2 hunks)packages/nuxt/src/core/templates.ts(2 hunks)packages/schema/src/index.ts(1 hunks)packages/schema/src/types/module.ts(3 hunks)test/fixtures/basic-types/config-types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/schema/src/index.ts
- packages/kit/test/module.test.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/config-types.tspackages/schema/src/types/module.tspackages/kit/src/module/install.tspackages/nuxt/src/core/templates.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/kit/src/module/install.ts
🧬 Code graph analysis (1)
packages/kit/src/module/install.ts (4)
packages/schema/src/types/module.ts (3)
NuxtModule(99-129)ModuleOptions(34-34)ModuleMeta(6-31)packages/schema/src/types/nuxt.ts (1)
Nuxt(89-116)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)
⏰ 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). (17)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, webpack, default, manifest-on, 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, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, default, 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, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: test-benchmark
- GitHub Check: test-size
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: code
🔇 Additional comments (1)
packages/nuxt/src/core/templates.ts (1)
288-293: EnsureModuleDependencyMetaimport exists in both augmentation blocks.You added the import; this looks correct.
Also applies to: 301-306
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/kit/src/module/install.ts (6)
18-19: MakemodulesDir→ URL handling OS‑agnostic (Windows-safe).Use a cross-platform trailing matcher instead of
'/node_modules/'literals.@@ -const NODE_MODULES_RE = /[/\\]node_modules[/\\]/ +const NODE_MODULES_RE = /[/\\]node_modules[/\\]/ +const NODE_MODULES_TRAIL_RE = /[\\/](?:node_modules)[\\/]?$/ // also used when mapping to URL @@ - from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(/\/node_modules\/?$/, '/'))), + from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(NODE_MODULES_TRAIL_RE, '/'))), @@ - from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(/\/node_modules\/?$/, '/'))), + from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(NODE_MODULES_TRAIL_RE, '/'))),Also applies to: 190-193, 225-230
276-278: MakenormalizeModuleTranspilePathOS‑agnostic.Split on both separators.
export const normalizeModuleTranspilePath = (p: string) => { - return getDirectory(p).split('node_modules/').pop() as string + return getDirectory(p).split(/[/\\]node_modules[/\\]/).pop() as string }
89-91: Fix inverted optional-dependency check (auto-install currently skips required deps).Required deps (
optional === false) should be auto-installed; skip only whenoptional === true.- if (value.optional === false) { + if (value.optional === true) { continue }
100-103: Prevent injecting the literals "string"/"function" intoresolvedModulePaths.The current fallback adds the result of
typeofwhenresolvedPathis falsy.- const path = resolvedModule.resolvedPath || typeof resolvedModule.module - if (typeof path === 'string') { - resolvedModulePaths.add(path) - } + const p = resolvedModule.resolvedPath || (typeof resolvedModule.module === 'string' ? resolvedModule.module : undefined) + if (p) { + resolvedModulePaths.add(p) + }
195-199: Don’t claim resolution succeeded when it didn’t.Keep
resolvedPathundefined ifresolveModulePathfails; returning the alias hides failures.return { module, - resolvedPath: modPath || modAlias, + resolvedPath: modPath || undefined, options, }
73-79: Guardfrompaths forreadPackageJSON(avoidundefinedand non-null assertions).Prevents passing invalid entries to
pkg-types.- const pkg = await readPackageJSON(name, { from: [res.resolvedModulePath!, ...nuxt.options.modulesDir] }).catch(() => null) + const from = [res.resolvedModulePath, ...nuxt.options.modulesDir].filter(Boolean) as string[] + const pkg = await readPackageJSON(name, { from }).catch(() => null)
🧹 Nitpick comments (2)
packages/kit/src/module/install.ts (2)
45-47: Avoidfalseentries ininlineConfigKeys; use an async mapper and filter.Prevents booleans from entering the Set and simplifies reasoning.
- const inlineConfigKeys = new Set( - await Promise.all([...modulesToInstall].map(([mod]) => typeof mod !== 'string' && Promise.resolve(mod.getMeta?.())?.then(r => r?.configKey))), - ) + const inlineConfigKeys = new Set( + (await Promise.all( + [...modulesToInstall].map(async ([mod]) => + typeof mod !== 'string' ? (await mod.getMeta?.())?.configKey : undefined, + ), + )).filter(Boolean) as string[], + )
117-119: Report all dependency issues, not just the last one.Aggregate errors for better DX during multi-dep resolution.
- if (error) { - throw error - } + if (error) { + // Optionally collect and throw AggregateError if multiple issues arise. + throw error + }If you want, I can provide a full patch converting this flow to use
AggregateErrorand pushing each failure instead of overwriting the same variable.
📜 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/kit/src/module/install.ts(4 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/kit/src/module/install.ts
🧠 Learnings (2)
📚 Learning: 2024-11-28T21:22:40.496Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#29661
File: packages/kit/src/template.ts:227-229
Timestamp: 2024-11-28T21:22:40.496Z
Learning: In `packages/kit/src/template.ts`, when updating the `EXTENSION_RE` regular expression for TypeScript configuration, avoid using patterns like `(\.\w+)+$` as they can result in catastrophic backtracking.
Applied to files:
packages/kit/src/module/install.ts
📚 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/kit/src/module/install.ts
🧬 Code graph analysis (1)
packages/kit/src/module/install.ts (4)
packages/schema/src/types/module.ts (3)
NuxtModule(99-129)ModuleOptions(34-34)ModuleMeta(6-31)packages/schema/src/types/config.ts (2)
NuxtOptions(81-93)NuxtConfig(49-66)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/nuxt/src/core/nuxt.ts (1)
553-553: Correct argument passed to installModules.installModules now receives resolvedModulePaths (not watch paths), fixing de-duplication.
packages/kit/src/module/install.ts (4)
226-231: Use the same Windows-safe normalisation when resolving module URLs.Mirror the cross-platform trailing node_modules handling here.
- const src = resolveModuleURL(nuxtModule, { - from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(/\/node_modules\/?$/, '/'))), + const src = resolveModuleURL(nuxtModule, { + from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(/[\\/](?:node_modules)[\\/]?$/, '/'))), suffixes: ['nuxt', 'nuxt/index', 'module', 'module/index', '', 'index'], extensions: ['.js', '.mjs', '.cjs', '.ts', '.mts', '.cts'], })
277-279: Normalise transpile path in an OS-agnostic way.Split on both separators; current split('node_modules/') fails on Windows.
-export const normalizeModuleTranspilePath = (p: string) => { - return getDirectory(p).split('node_modules/').pop() as string -} +export const normalizeModuleTranspilePath = (p: string) => { + return getDirectory(p).split(/[/\\]node_modules[/\\]/).pop() as string +}
45-47: Fix unsafe chaining on optional getMeta.Promise.resolve(mod.getMeta?.())?.then(...) can invoke .then on undefined. Use an async mapper.
Apply:
- const inlineConfigKeys = new Set( - await Promise.all([...modulesToInstall].map(([mod]) => typeof mod !== 'string' && Promise.resolve(mod.getMeta?.())?.then(r => r?.configKey))), - ) + const inlineConfigKeys = new Set( + await Promise.all( + [...modulesToInstall].map(async ([mod]) => + typeof mod !== 'string' ? (await mod.getMeta?.())?.configKey : undefined, + ), + ), + )
188-201: Don’t treat unresolved aliases as resolved paths; make modulesDir root URL Windows-safe.Setting resolvedPath to modAlias disguises unresolved modules and can break de-duplication. Also, the /node_modules/?$ regex is POSIX-only.
const modPath = resolveModulePath(modAlias, { try: true, - from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(/\/node_modules\/?$/, '/'))), + from: nuxt.options.modulesDir.map(m => directoryToURL(m.replace(/[\\/](?:node_modules)[\\/]?$/, '/'))), suffixes: ['nuxt', 'nuxt/index', 'module', 'module/index', '', 'index'], extensions: ['.js', '.mjs', '.cjs', '.ts', '.mts', '.cts'], }) return { module, - resolvedPath: modPath || modAlias, + resolvedPath: modPath || undefined, options, }
🧹 Nitpick comments (5)
packages/nuxt/src/core/nuxt.ts (2)
900-907: Minor: avoid duplication in module-resolution loop.This block is nearly identical to the one below (Lines 931–938). Consider extracting a small helper (e.g., registerResolvedModuleIfUnique) to keep logic in one place.
931-938: Same refactor opportunity as above.The dedupe + Set update logic could be centralised to reduce future drift between the two loops.
packages/kit/src/module/install.ts (3)
74-80: Guard readPackageJSON “from” paths and drop the non-null assertion.Avoid passing undefined and remove the misleading !.
- const resolvePaths = [res.resolvedModulePath!, ...nuxt.options.modulesDir].filter(Boolean) - const pkg = await readPackageJSON(name, { from: resolvePaths }).catch(() => null) + const from = [res.resolvedModulePath, ...nuxt.options.modulesDir].filter(Boolean) as string[] + const pkg = await readPackageJSON(name, { from }).catch(() => null)
118-120: Aggregate multiple dependency errors for better DX.Currently only the last error is thrown. Consider collecting all messages and throwing a combined error to surface all issues at once.
321-332: Avoid duplicate pushes to transpile and modulesDir.Consider guarding before push to reduce array growth and duplicate work.
- nuxt.options.build.transpile.push(normalizeModuleTranspilePath(moduleRoot)) + const transpileEntry = normalizeModuleTranspilePath(moduleRoot) + if (!nuxt.options.build.transpile.includes(transpileEntry)) { + nuxt.options.build.transpile.push(transpileEntry) + } @@ - if (moduleRoot !== moduleToInstall && !localLayerModuleDirs.some(dir => directory.startsWith(dir))) { - nuxt.options.modulesDir.push(join(moduleRoot, 'node_modules')) - } + if (moduleRoot !== moduleToInstall && !localLayerModuleDirs.some(dir => directory.startsWith(dir))) { + const nm = join(moduleRoot, 'node_modules') + if (!nuxt.options.modulesDir.includes(nm)) { + nuxt.options.modulesDir.push(nm) + } + }
📜 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 (3)
packages/kit/src/module/install.ts(4 hunks)packages/nuxt/src/core/nuxt.ts(7 hunks)packages/nuxt/src/core/templates.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/src/core/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:
packages/kit/src/module/install.tspackages/nuxt/src/core/nuxt.ts
🧠 Learnings (2)
📚 Learning: 2024-11-28T21:22:40.496Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#29661
File: packages/kit/src/template.ts:227-229
Timestamp: 2024-11-28T21:22:40.496Z
Learning: In `packages/kit/src/template.ts`, when updating the `EXTENSION_RE` regular expression for TypeScript configuration, avoid using patterns like `(\.\w+)+$` as they can result in catastrophic backtracking.
Applied to files:
packages/kit/src/module/install.ts
📚 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/kit/src/module/install.tspackages/nuxt/src/core/nuxt.ts
🧬 Code graph analysis (2)
packages/kit/src/module/install.ts (3)
packages/schema/src/types/module.ts (3)
NuxtModule(99-129)ModuleOptions(34-34)ModuleMeta(6-31)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)
packages/nuxt/src/core/nuxt.ts (1)
packages/kit/src/module/install.ts (2)
installModules(34-145)resolveModuleWithOptions(171-201)
⏰ 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: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: typecheck (windows-latest, bundler)
🔇 Additional comments (6)
packages/nuxt/src/core/nuxt.ts (4)
9-9: Importing kit’s install/resolution APIs looks correct.No issues spotted with the new imports.
424-427: Good: propagate resolvedModulePaths and watch local module files.Destructure now returns resolvedModulePaths and you add watchedModulePaths to nuxt.options.watch. Solid.
666-667: Efficient restart on local module changes.Using watchedModulePaths.has(path) avoids regex scans and correctly hard-restarts.
944-946: Return shape alignment LGTM.Including resolvedModulePaths in resolveModules’ return keeps call sites simple.
packages/kit/src/module/install.ts (2)
90-92: Optional dependency handling is correct.Skipping auto-install when optional === true matches the intended semantics.
122-139: Confirm override/default merge precedence.defu(...overrides, nuxt.options[configKey], ...defaults) gives precedence to earlier overrides, then user config, then defaults. If this is the intended policy across multiple dependants, please confirm; otherwise, we might need a deterministic ordering (e.g. by dependency declaration order or module name).
Perhaps as a follow-up, but I still think this would be a good idea. |
🔗 Linked issue
resolves #31674
📚 Description
this adds the possibility for modules to:
peerDependencieswhat this does not do (and I have no plans to do) is build a module graph and allow modules to change the order in which they run, which I regard as an architectural mistake.
modules which want to allow other modules to integrate with them should use hooks which they then call in
modules:done(after all modules have run and had a chance to register callbacks for these hooks)we also deprecate
installModuleas it has some shortcomings: