Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: .isFile()/.isDir()/.isLink()/.isSymlink() #998

Open
queengooborg opened this issue Feb 24, 2023 · 3 comments
Open

Feature request: .isFile()/.isDir()/.isLink()/.isSymlink() #998

queengooborg opened this issue Feb 24, 2023 · 3 comments

Comments

@queengooborg
Copy link

In mdn/yari, we use the following code to check to see if a file path is a directory:

if ((await fs.stat(<path>)).isDirectory()) {
  // ...
}

It would be awesome for fs-extra to have a shortcut:

if (await fse.isDir(<path>)) {
  // ...
}
@RyanZim
Copy link
Collaborator

RyanZim commented Feb 24, 2023

I'm not completely closed to this idea. However, here's a couple of arguments against it:

  1. It's not obvious whether these methods should use 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.

  2. 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.

@queengooborg
Copy link
Author

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?

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 27, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants