Open queengooborg opened 1 year ago
I'm not completely closed to this idea. However, here's a couple of arguments against it:
fs.stat
or fs.lstat
. fs.lstat
would obviously be used for isSymlink()
. However, if you're calling isDir()
, it's not clear whether you want to know whether the actual path is a directory, or if you want to know whether whatever the path points to (i.e. path is a symlink) is a directory. Your example code uses fs.stat
; however the mere existence of an analogous isSymlink()
method would suggest that isDir
would use fs.lstat
, though you probably don't want that.Each of these separate methods calls stat
/lstat
itself. This is fine if you're only using one of these methods. However, that's inefficient if you're using multiple calls on the same file:
if (await fse.isDir(path)) {
// do something...
} else if (await fse.isFile(path)) {
// do something else...
} else {
// handle this edge case...
}
It would be more performant in that case to do:
const stats = await fs.lstat(path)
if (stats.isDirectory()) {
// do something...
} else if (stats.isFile()) {
// do something else...
} else {
// handle this edge case...
}
Doing an explicit stats call makes the performance refactor an obvious choice; I'm afraid that these "sugar" methods will obscure the process and lead to inefficient code.
isLink()
is sort of an interesting idea; fs.Stats
doesn't have such a method; you have to manually check stats.nlinks > 1
.
Thanks for the quick response, I hadn't thought about this obfuscation! I was thinking primarily of .isFile()
and .isDir()
, but I saw that .ensureLink()
and .ensureSymlink()
existed alongside .ensureDir()
and .ensureFile()
, so I added those to the list. (Side note: what is the difference between .ensureLink()
and .ensureSymlink()
?)
One possible way to help make the end result more explicit could potentially be a boolean symlinkAllowed
option (or similar name), defaulting to true
; if set to false
, the file/directory must not be a symlink. I imagine that most developers are checking to see if the file/directory exists because they intend to read or write to said file/directory, similar to why they may use .ensureFile()
or .ensureDir()
.
Another option I could see is to create a .getType()
method, which would offer a more performant solution than the above methods (though I'd still love to see .isFile()
/.isDir()
). I imagine it could return something like so:
const ft = await fse.getType(path);
// {
// type: 'file' | 'dir' | null,
// isLink: true | false
// }
if (ft.type === 'dir') {
// Handle directory
else if (ft.type === 'file') {
// Handle file
if (ft.isLink) {
// Handle symlink to file
} else {
// Handle non-symlink file
}
} else if (ft.type === null) {
// Path doesn't exist!
}
WDYT?
Side note: what is the difference between .ensureLink() and .ensureSymlink()?
.ensureLink()
creates a hard link; .ensureSymlink()
creates a symbolic link.
One possible way to help make the end result more explicit could potentially be a boolean symlinkAllowed option
Generally speaking, unnamed boolean options aren't a good idea. If we have a named option, that results in a function call that is longer than the vanilla function call:
await fse.isFile(path, { symlinkAllowed: false })
(await fs.stat(path)).isFile()
I imagine that most developers are checking to see if the file/directory exists because they intend to read or write to said file/directory
Generally speaking, this is an anti-pattern. The Node.js docs state:
Using fs.stat() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Instead, user code should open/read/write the file directly and handle the error raised if the file is not available.
Another option I could see is to create a .getType() method
How is getType
not just a reinvention of fs.stat
with a slightly different API?
In mdn/yari, we use the following code to check to see if a file path is a directory:
It would be awesome for
fs-extra
to have a shortcut: