feat(module): add module remove command#1306
Conversation
commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
=======================================
Coverage ? 55.43%
=======================================
Files ? 50
Lines ? 1398
Branches ? 400
=======================================
Hits ? 775
Misses ? 513
Partials ? 110 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR implements the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (2)
packages/nuxi/test/unit/commands/module/remove.spec.ts (2)
27-53: 💤 Low value
runCommandandfetchModulesspies are set at describe scope and never clearedBoth spies accumulate call counts across tests, and
fetchModuleswon't re-read its mock implementation if it needs to change per-test. Since all four tests currently rely on the same stubs this doesn't cause failures, but it's fragile. Move the spy setup intobeforeEach(or call.mockClear()/.mockReset()there alongsideupdateConfigandremoveDependency).♻️ Proposed fix
-describe('module remove', () => { - vi.spyOn(runCommands, 'runCommand').mockImplementation(vi.fn()) - vi.spyOn(utils, 'fetchModules').mockResolvedValue([...]) - - beforeEach(() => { - updateConfig.mockClear() - removeDependency.mockClear() - }) +describe('module remove', () => { + beforeEach(() => { + updateConfig.mockClear() + removeDependency.mockClear() + vi.spyOn(runCommands, 'runCommand').mockImplementation(vi.fn()) + vi.spyOn(utils, 'fetchModules').mockResolvedValue([...]) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxi/test/unit/commands/module/remove.spec.ts` around lines 27 - 53, The spies for runCommands.runCommand and utils.fetchModules are created at describe scope and retain state between tests; move their setup into a beforeEach (or call .mockClear()/.mockReset() there) so each test gets a fresh spy and fetchModules can be re-mocked per-test; also ensure you clear mocks for related functions like updateConfig and removeDependency in the same beforeEach to avoid accumulated call counts and stale implementations when tests change behavior.
7-7: 🏗️ Heavy lift
updateConfigmock bypassesonUpdate— the config-removal logic is entirely untestedThe mock resolves immediately without ever calling the
onUpdatecallback. As a result,removedFromConfigstays empty in every test,readModuleName, the reverse-splice loop (lines 159-166 ofremove.ts), and themultiselectpicker path are never exercised. The tests only validate the uninstall-from-disk flow.To test the config-update logic, the mock should capture and invoke the callback:
♻️ Proposed fix
-const updateConfig = vi.fn(() => Promise.resolve()) +const updateConfig = vi.fn(async (opts: { onUpdate?: (config: any) => Promise<void>, onCreate?: () => boolean | void }) => { + if (opts.onUpdate) { + await opts.onUpdate({ modules: ['@nuxt/content'] }) + } +})Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxi/test/unit/commands/module/remove.spec.ts` at line 7, The test's updateConfig mock currently resolves without invoking the module removal callback, so the config-removal path (onUpdate callback) is never exercised; change the vi.fn mock for updateConfig to accept the same args as the real function and call the provided onUpdate callback with a simulated config and return its result so removedFromConfig gets populated; specifically, update the mock referenced as updateConfig in the spec to call the onUpdate param (triggering readModuleName, the reverse-splice logic in remove.ts, and the multiselect picker path) and return the resulting Promise so the tests exercise both config-update and disk-uninstall flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nuxi/src/commands/module/remove.ts`:
- Around line 127-166: The code assumes config.modules is always an array
causing a TypeError when it's undefined; ensure safe handling by normalizing
config.modules to an array before reading or mutating it (e.g., if
(!Array.isArray(config.modules)) config.modules = []), then build the present
list with readModuleName from config.modules, use modules.length / multiselect
logic as-is, and safely iterate backwards over config.modules to splice and push
into removedFromConfig; this keeps the rest of the flow (multiselect, isCancel,
toRemove, readModuleName, removedFromConfig) unchanged while preventing
undefined access.
- Around line 153-166: The removal logic compares resolved npm names in toRemove
to raw config entries from readModuleName, so alias entries like 'content' won't
match and remain in nuxt.config after uninstall; update removeModules to compare
resolved names by either (A) resolving each config entry with resolveModule (or
using modulesDB/installedNames) before checking (i.e., call resolveModule on the
value returned by readModuleName and compare that resolved name to toRemove) or
(B) populate toRemove with both the original and resolved package names so the
existing loop that uses readModuleName finds matches; update references to
toRemove, readModuleName, resolveModule, removeModules, and
modulesDB/installedNames accordingly.
- Around line 235-244: The removeDependency rejection is currently only logged
and the function returns true, causing the caller to still run prepare; change
the error handling in the removeDependency call so that after logging you either
re-throw the error or return false to propagate failure to the caller (so caller
honor prepare skip when ctx.args.skipUninstall is false), and update the calling
code that checks the boolean result to skip calling prepare when false; also
confirm nypm.removeDependency's signature — if it does not accept string[] then
iterate over allToRemove and call removeDependency(name, options) for each entry
(or use Promise.all on per-name calls) instead of passing the array. Ensure
references to removeDependency, prepare, ctx.args.skipUninstall and allToRemove
are updated accordingly.
---
Nitpick comments:
In `@packages/nuxi/test/unit/commands/module/remove.spec.ts`:
- Around line 27-53: The spies for runCommands.runCommand and utils.fetchModules
are created at describe scope and retain state between tests; move their setup
into a beforeEach (or call .mockClear()/.mockReset() there) so each test gets a
fresh spy and fetchModules can be re-mocked per-test; also ensure you clear
mocks for related functions like updateConfig and removeDependency in the same
beforeEach to avoid accumulated call counts and stale implementations when tests
change behavior.
- Line 7: The test's updateConfig mock currently resolves without invoking the
module removal callback, so the config-removal path (onUpdate callback) is never
exercised; change the vi.fn mock for updateConfig to accept the same args as the
real function and call the provided onUpdate callback with a simulated config
and return its result so removedFromConfig gets populated; specifically, update
the mock referenced as updateConfig in the spec to call the onUpdate param
(triggering readModuleName, the reverse-splice logic in remove.ts, and the
multiselect picker path) and return the resulting Promise so the tests exercise
both config-update and disk-uninstall flows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69293820-fddb-4912-b24a-6ce800fe37f7
📒 Files selected for processing (3)
packages/nuxi/src/commands/module/index.tspackages/nuxi/src/commands/module/remove.tspackages/nuxi/test/unit/commands/module/remove.spec.ts
|
we've deprecated wdyt @atinux? |
|
I guess it could be |
|
I'll change it to |
|
Good for me @Flo0806 |
|
|
danielroe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nuxi/test/unit/commands/module/remove.spec.ts (1)
104-120: ⚡ Quick winAdd cancellation-path tests for interactive prompts.
Please add explicit tests where
multiselectandconfirmreturnSymbol.for('clack:cancel')(or mocked cancel symbol) to verifysetupaborts and skips uninstall/update side effects on those branches.Also applies to: 165-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxi/test/unit/commands/module/remove.spec.ts` around lines 104 - 120, Add new test cases to handle cancellation paths in the remove command setup. Create additional test blocks where multiselect is mocked to return Symbol.for('clack:cancel') (and similar for confirm when applicable) to verify that when users cancel the interactive prompt, the setup method returns early without executing the removeDependency call or other side effects like updateConfig. These cancellation tests should follow the same pattern as the existing test structure but verify the abort behavior instead of the successful removal behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nuxi/src/commands/module/_utils.ts`:
- Around line 183-186: The forwardCommandArgs function currently serializes
undefined and null values into command flags like --logLevel=undefined, which is
invalid. Add an additional filter condition in the chain to exclude entries
where the value (v) is undefined or null before the map operation that formats
the arguments as strings. This ensures only valid option values are forwarded to
runCommand invocations.
---
Nitpick comments:
In `@packages/nuxi/test/unit/commands/module/remove.spec.ts`:
- Around line 104-120: Add new test cases to handle cancellation paths in the
remove command setup. Create additional test blocks where multiselect is mocked
to return Symbol.for('clack:cancel') (and similar for confirm when applicable)
to verify that when users cancel the interactive prompt, the setup method
returns early without executing the removeDependency call or other side effects
like updateConfig. These cancellation tests should follow the same pattern as
the existing test structure but verify the abort behavior instead of the
successful removal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d777d25-340a-4298-8530-d0e93fb8b472
📒 Files selected for processing (4)
packages/nuxi/src/commands/module/_utils.tspackages/nuxi/src/commands/module/add.tspackages/nuxi/src/commands/module/remove.tspackages/nuxi/test/unit/commands/module/remove.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxi/src/commands/module/remove.ts
| export function forwardCommandArgs(args: Record<string, unknown>): string[] { | ||
| return Object.entries(args) | ||
| .filter(([k]) => k in cwdArgs || k in logLevelArgs) | ||
| .map(([k, v]) => `--${k}=${v}`) |
There was a problem hiding this comment.
Avoid forwarding undefined/null option values in chained args.
forwardCommandArgs currently serializes absent values into flags like --logLevel=undefined, which can leak invalid values into runCommand(...) invocations.
Suggested patch
export function forwardCommandArgs(args: Record<string, unknown>): string[] {
return Object.entries(args)
- .filter(([k]) => k in cwdArgs || k in logLevelArgs)
- .map(([k, v]) => `--${k}=${v}`)
+ .filter(([k, v]) =>
+ (Object.prototype.hasOwnProperty.call(cwdArgs, k)
+ || Object.prototype.hasOwnProperty.call(logLevelArgs, k))
+ && v !== undefined
+ && v !== null,
+ )
+ .map(([k, v]) => `--${k}=${String(v)}`)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuxi/src/commands/module/_utils.ts` around lines 183 - 186, The
forwardCommandArgs function currently serializes undefined and null values into
command flags like --logLevel=undefined, which is invalid. Add an additional
filter condition in the chain to exclude entries where the value (v) is
undefined or null before the map operation that formats the arguments as
strings. This ensures only valid option values are forwarded to runCommand
invocations.
4 participants
🔗 Linked issue
Resolves: #1184
📚 Description
Added a
module removecommand to remove modules from Nuxtsmodulesarray, including uninstallation from package.json (along with peer dependencies, provided they are not required by other packages).Possible params:
skipConfig,skipUninstallTests
Added tests , similar to add command