readdir(p, { withFileTypes: true }) seemingly returns wrong type

[apparebit]

• Version: v13.1.0
• Platform: Darwin White-Star.local 18.7.0 Darwin Kernel Version 18.7.0: Sat Oct 12 00:02:19 PDT 2019; root:xnu-4903.278.12~1/RELEASE_X86_64 x86_64
• Subsystem: fs

When listing a directory with readdir and withFileTypes, at least the promisified version of fs.readdir returns the wrong type for entities that are symbolic links to directories:

const { basename, join } = require('path');
const { readdir, lstat, stat, symlink, unlink } = require('fs').promises;
const LINK_NAME = basename(__filename) + '-link';
const LINK_PATH = join(__dirname, LINK_NAME);

const inspectEntity = (label, entity) => {
  console.log(label, (entity.isDirectory() ? 'dir' : '') +
    (entity.isFile() ? 'file' : '') +
    (entity.isSymbolicLink() ? 'symlink' : ''));
};

(async function main() {
  try {
    await symlink(__dirname, LINK_PATH, 'dir');

    for (const entity of await readdir(__dirname, { withFileTypes: true })) {
      if (entity.name === LINK_NAME) inspectEntity('readdir', entity);
    }
    inspectEntity('lstat', await lstat(LINK_PATH));
    inspectEntity('stat', await stat(LINK_PATH));

    await unlink(LINK_PATH);
  } catch (x) {
    console.error(x.stack);
  }
})();

Running the above script yields the following on my machine:

readdir file
lstat symlink
stat dir

Since the file system entity in question is a symbolic link to a directory, I would expect the code to either report a symlink or a directory, depending on whether the implementation of readdir uses stat or lstat under the covers. But in neither case would I expect a file to be reported. Yet that's exactly what's happening on my machine. Am I missing something or is this a genuine bug?

Additional testing with a second machine suggests that this may be related to remote file system usage. When running the test locally on a local disk, readdir reports a symlink (yay!). That stands in contrast to my original testing was performed on a remotely mounted file system.

As an aside, after reading a number of old issues before filing this issue, I am guessing that readdir uses stat not lstat under the covers. It would seem more consistent with the rest of the API. Yet the documentation has no disclaimer on dirent.isSymbolicLink() as it has on stats.isSymbolicLink(). Either way, being more upfront about readdir's behavior {withFileTypes: true} would go a long way towards a better developer experience. I'm happy to submit a PR for that—once I understand what's going on above.

Even with better documentation though, some developers will be disappointed when whatever readdir uses does not meet their use case. So I am also wondering whether configurability of stat/lstat for readdir withFileTypes: true would be desirable in the long term. If that suggestion falls under the heading "long-term costs of exposing things from core," I apologize. I'm gonna practice writing code without using withFileTypes for certain tonight. 😜

[addaleax]

@apparebit I think this is ultimately dependent on what the platform’s/file system’s scandir() function reports to libuv. @nodejs/platform-macos

As an aside, after reading a number of old issues before filing this issue, I am guessing that readdir uses stat not lstat under the covers. It would seem more consistent with the rest of the API. Yet the documentation has no disclaimer on dirent.isSymbolicLink() as it has on stats.isSymbolicLink(). Either way, being more upfront about readdir's behavior {withFileTypes: true} would go a long way towards a better developer experience. I'm happy to submit a PR for that—once I understand what's going on above.

libuv or Node.js calls neither stat() nor lstat(), it mostly relies on scandir() reporting the right thing when using fs.readdir(); however, my Linux man page for scandir() suggests that it should be using lstat().

Feel free to submit a PR, but I really feel like in the end the result might just be something along the lines “The type of the entry may be dependent on the file system and operating system.”

Even with better documentation though, some developers will be disappointed when whatever readdir uses does not meet their use case. So I am also wondering whether configurability of stat/lstat for readdir withFileTypes: true would be desirable in the long term. If that suggestion falls under the heading "long-term costs of exposing things from core," I apologize. I'm gonna practice writing code without using withFileTypes for certain tonight. stuck_out_tongue_winking_eye

I think any alternative would require calling lstat() on all directory entries manually … we do use that approach, but only when the OS reports back an unknown file type, because the performance impact is likely pretty noticeable.