chore(deps): remove rimraf devDependency#5775
Conversation
Replace `rimraf.sync()` with native `fs.rmSync()` in the init integration
test. `fs.rmSync` with `{ recursive: true, force: true }` has been stable
since Node.js 14.14.0.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
👋 Hi @roli-lpci, thanks for the pull request! A scan flagged a concern with it. Could you please take a look? [pr-task-completion] This PR's body is missing
Repositories often provide a set of tasks that pull request authors are expected to complete. Those tasks should be marked as completed with a
|
|
Thanks! I'll quickly note that all maintainers are pretty busy this month, but I'll get to this next weekend (if not this one!) |
There was a problem hiding this comment.
Pull request overview
Removes the rimraf devDependency and replaces its usages with Node’s built-in fs.rmSync, reducing external tooling for cleanup operations.
Changes:
- Replaced
rimraf.sync()withfs.rmSync(..., { recursive: true, force: true })in an integration test cleanup hook - Replaced
rimraf-based npm scripts withnode -ecleanup one-liners - Removed
rimraffromdevDependenciesand updatedpackage-lock.jsonaccordingly
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/integration/init.spec.js | Uses fs.rmSync for temp directory cleanup instead of rimraf |
| package.json | Removes rimraf devDependency; updates clean and test-coverage-clean scripts to use fs.rmSync |
| package-lock.json | Drops top-level rimraf package; lockfile regenerated to reflect dependency graph |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "scripts": { | ||
| "build": "rollup -c ./rollup.config.mjs", | ||
| "clean": "rimraf mocha.js mocha.js.map", | ||
| "clean": "node -e \"const fs=require('fs');['mocha.js','mocha.js.map'].forEach(f=>{try{fs.rmSync(f)}catch{}})\"", |
There was a problem hiding this comment.
The new clean script swallows all fs.rmSync errors, which means npm run clean will exit successfully even if deletion fails for reasons other than a missing file (e.g., permissions/locked file). This can leave stale artifacts around and make failures harder to diagnose. Consider using fs.rmSync(path, { force: true }) (no try/catch needed for non-existent paths) or only suppressing ENOENT and rethrowing other errors.
| "clean": "node -e \"const fs=require('fs');['mocha.js','mocha.js.map'].forEach(f=>{try{fs.rmSync(f)}catch{}})\"", | |
| "clean": "node -e \"const fs=require('fs');['mocha.js','mocha.js.map'].forEach(f=>fs.rmSync(f,{force:true}))\"", |
There was a problem hiding this comment.
I like this suggestion, @roli-lpci let me know your thoughts :)
| "test-browser:webpack-compat": "webpack --mode development --config ./test/browser-specific/fixtures/webpack/webpack.config.js", | ||
| "test-browser": "run-s clean build test-browser:*", | ||
| "test-coverage-clean": "rimraf .nyc_output coverage", | ||
| "test-coverage-clean": "node -e \"['.nyc_output','coverage'].forEach(d=>{try{require('fs').rmSync(d,{recursive:true,force:true})}catch{}})\"", |
There was a problem hiding this comment.
test-coverage-clean catches and ignores any rmSync failure, so the script can report success even when .nyc_output/coverage couldn't be removed. Since rmSync is already called with { recursive: true, force: true } (which avoids ENOENT), consider removing the blanket try/catch (or only ignoring ENOENT) so real cleanup failures still fail the script.
| "test-coverage-clean": "node -e \"['.nyc_output','coverage'].forEach(d=>{try{require('fs').rmSync(d,{recursive:true,force:true})}catch{}})\"", | |
| "test-coverage-clean": "node -e \"['.nyc_output','coverage'].forEach(d=>require('fs').rmSync(d,{recursive:true,force:true}))\"", |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5775 +/- ##
=======================================
Coverage 88.74% 88.74%
=======================================
Files 66 66
Lines 4744 4744
Branches 976 976
=======================================
Hits 4210 4210
Misses 534 534 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
-1 from me. Using node -e "..." in scripts is a lot of work and prone to error. The rimraf devDependency does the job well.
The e18e cleanup initiative is great for packages shipped to end-users. But rimraf is just a devDependency for Mocha. This does nothing for end-users and at best would be a fraction of a percent cleaner for Mocha developers. I don't think this PR is worth consideration.
PR Checklist
rimraf(ecosystem-issues#178).status: accepting prsOverview
Remove
rimrafdevDependency, replace with nativefs.rmSync(stable since Node.js 14.14).Changes
test/integration/init.spec.js— Replacerimraf.sync(tmpdir)withfs.rmSync(tmpdir, { recursive: true, force: true })package.json— ReplacerimrafCLI incleanandtest-coverage-cleanscripts with cross-platformnode -eone-liners; removerimraffrom devDependenciespackage-lock.json— RegeneratedTesting
npm run clean— works correctlynpm run test-coverage-clean— works correctlytest/integration/init.spec.js— 3/3 passing