feat(nuxi): add module remove command#1306
Conversation
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
=======================================
Coverage ? 52.87%
=======================================
Files ? 50
Lines ? 1375
Branches ? 399
=======================================
Hits ? 727
Misses ? 529
Partials ? 119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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