fix: added check for dry-run #7274
Conversation
added a check for dry-run for `npm ci` so that node_modules won't be deleted closes #7239
lib/commands/ci.js Outdated
| if (!this.npm.flatOptions.dryRun) { | ||
| // Only remove node_modules after we've successfully loaded the virtual | ||
| // tree and validated the lockfile | ||
| await this.npm.time('npm-ci:rm', async () => { | ||
| const path = `${where}/node_modules` | ||
| // get the list of entries so we can skip the glob for performance | ||
| const entries = await fs.readdir(path, null).catch(er => []) | ||
| return Promise.all(entries.map(f => fs.rm(`${path}/${f}`, { force: true, recursive: true }))) | ||
| }) | ||
| } |
There was a problem hiding this comment.
maybe it'd be better if, when a dry run, it checked that npm has permissions to rimraf node_modules?
There was a problem hiding this comment.
I am not sure what you mean by 'permissions'. Sorry this is my first time contributing to npm.
There was a problem hiding this comment.
It wasn't checking for any permissions before so I am confused.
There was a problem hiding this comment.
You’re right, but i meant it’d be useful and match the “dry run” semantics
There was a problem hiding this comment.
Checking permissions in node is mostly done by trying the operation and then reporting to the user if it failed. It's even discouraged for things like fs.access to be used for this purpose. With that in mind I think it's fine for dry-run to simply tell the user what it would try to do. It is not meant to be a test of if npm will eventually be able to succeed. Many other things could go wrong (i.e. running out of disk space).
This PR appears fine as-is (I am not done fully reviewing it but at a glance it appears mostly on the right track).
There was a problem hiding this comment.
presumably deleting files wouldn't cause disk space to run out :-)
def that enhancement could be done as a follow through regardless
| I don't know if this is a problem for this PR to solve, but technically this dry run is meaningless because Arborist isn't being presented with the same scenario as a true This doesn't feel like a blocker, as this is better than before. |
jumoel left a comment
jumoel left a comment There was a problem hiding this comment.
This is a great, thanks for fixing it!
4 participants
added a check for dry-run for
npm ciso that node_modules won't be deleted.npm ci --dry-runcurrently removes the node_modules folder even though the docs say that dry-run, "Indicates that you don't want npm to make any changes and that it should only report what it would have done".This commit adds a check for dryRun in the npm.flatOptions object which will be true if
--dry-runis added on the cli or ifdry-run=trueis set in .npmrccloses #7239