fix(deps): remove del dependency #4017
Conversation
|
I don't see any deprecation warnings on CI, but I could use more eyes to be safe. :) |
src/lib/fs.js
Outdated
| @@ -40,7 +39,7 @@ const rmdirRecursiveAsync = async (path) => { | |||
| if (gte(NODE_VERSION, '14.14.0')) { | |||
| return await rm(path, { force: true, recursive: true }) | |||
| } | |||
| await del(path, { force: true }) | |||
| await rmdir(path, { force: true, recursive: true }) | |||
There was a problem hiding this comment.
Unlike fs.promises.rm() and del, fs.promises.rmdir() does not have a force option. Running it against a non-existing file throws ENOENT, but only starting from Node 16.0.0
The recursive option with fs.promises.rmdir() is deprecated. Starting from Node 16.0.0, using it prints an error message:
(node:24125) [DEP0147] DeprecationWarning: In future versions of Node.js, fs.rmdir(path, { recursive: true }) will be removed. Use fs.rm(path, { recursive: true }) instead
fs.promises.rm() requires Node 14.14.0.
We could possibly fix this by using a utility function along the lines of:
const { promises: fs } = require('fs')
export const removeDir = function (path) {
return fs.rm === undefined
? fs.rmdir(path, { recursive: true })
: fs.rm(path, { force: true, recursive: true })
}However, I am wondering if we should just wait to drop Node 12, and use fs.promises.rm() then? Just in case there were some subtle behavior differences between del, rmdir() and rm() like the one highlighted above. What do you think?
There was a problem hiding this comment.
Correct me if I'm wrong, since there's code that's using fs.rm if Node.js is >= 14.14.0, then this path will be followed only on Node.js >= 12.20 and < 14.14.0.
I can't talk about force, I'll take your word for it :)
There was a problem hiding this comment.
Oh, I did not look at the context, only the line of code, and failed to notice this was already inside a test helper with a Node.js version condition 🤦 Thanks for pointing this out.
This looks good then, except the force option can be removed since it does not exist. 👍
There was a problem hiding this comment.
I removed force but I'm unsure about the change since the 2 code paths don't seem to be equivalent without force...
There was a problem hiding this comment.
Yes, I'm wondering too.
fs.promises.rmdir() does not have a force option, so it would be a noop. It is not documented and seems to be absent from the nodejs/node codebase too.
When trying fs.promises.rmdir() locally with Node 12 with recursive: true, it appears to me that it is a noop when the directory does not exist, which would be good. However, if we're unsure, we also have the option of keeping del until we drop Node 12.
What are your thoughts on this? Which option do you think might be the best?
There was a problem hiding this comment.
I think the behavior should be the same, but I can't guarantee it. I'm fine with dropping this PR until Node.js 12.x support is dropped and you can clean up this file even more + do this in all of your project.
| @@ -40,7 +39,7 @@ const rmdirRecursiveAsync = async (path) => { | |||
| if (gte(NODE_VERSION, '14.14.0')) { | |||
| return await rm(path, { force: true, recursive: true }) | |||
| } | |||
| await del(path, { force: true }) | |||
| await rmdir(path, { recursive: true }) | |||
There was a problem hiding this comment.
This is breaking the behavior. It does not support the force argument as this is only supported in rm which is only avialable from 14.14.0
Which means if it would be used to delete a file it would fail:
https://nodejs.org/api/fs.html#fspromisesrmdirpath-options
Using fsPromises.rmdir() on a file (not a directory) results in the promise being rejected with an ENOENT error on Windows and an ENOTDIR error on POSIX.
To get a behavior similar to the rm -rf Unix command, use fsPromises.rm() with options { recursive: true, force: true }.
As our test coverage is sadly to low I would not do this change as we cannot predict the side effects. I would try to update the minimum node version as fast as possible to 14.14.0 to use rm.
There was a problem hiding this comment.
It appears that fs.promises.rmdir() does not throw ENOENT on missing directories in Node <14.14.0, but I agree with @lukasholzer this might be safer to wait until we drop Node 12. 👍
|
Closing for the aforementioned reasons. Just something more to do when Node.js 12.x is dropped :) |
|
Thanks for looking into this though @XhmikosR! ❤️ |
So, apparently there is already code for
rm. Testing to see if there are any deprecation warnings/errors.The
src/lib.fs.jsfile could be simplified later when Node.js 12.x support is dropped, removing semver and the conditionalrm.Also, if you have similar code in your other packages which cli depends one, it'd be nice to apply the same patch. If not, when Node.js 12.x support is dropped, you will be able to remove the del package from the other packages.
Refs #3941.
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #<replace_with_issue_number>
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)