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

The ‘recursive’ option for FileSystem.readDirectory does nothing on Node 18 #1801

Closed
strogonoff opened this issue Dec 28, 2023 · 3 comments · Fixed by #2458
Closed

The ‘recursive’ option for FileSystem.readDirectory does nothing on Node 18 #1801

strogonoff opened this issue Dec 28, 2023 · 3 comments · Fixed by #2458
Labels
bug Something isn't working platform

Comments

@strogonoff
Copy link
Contributor

strogonoff commented Dec 28, 2023

What version of Effect is running?

2.0.0-next.62

What steps can reproduce the bug?

Attempt using fs.readDirectory() with recursive option on a directory with nested directories and files.

What is the expected behavior?

The listing is recursive, or the option is not listed in docs in the first place.

What do you see instead?

The resulting listing is not recursive and no error is thrown, neither at compile nor at run time.

Additional information

To be clear, from my tests this is the same with vanilla Node 18’s fs.readdir(). I.e., it appears to be either a bug or misdocumentation in Node (I wonder if in this issue someone fat-finger added option in docs of older Node version where it is not supported). However, since this package appears to “proxy” Node docs the deficiency applies here as well.

I guess either:

  • The option should not be listed in docstring (if Node docs are mistaken, they should not be proxied)
  • Dropping Node 18 would fix it (assuming the option works in Node 20), though to be honest I am personally relying on Effect‘s Node 18 support currently…
  • The option is implemented in Effect/platform, to compensate for Node’s lack, so that it works as docs say
@strogonoff strogonoff added the bug Something isn't working label Dec 28, 2023
@fubhy fubhy transferred this issue from Effect-TS/platform Jan 1, 2024
@fubhy fubhy added the platform label Jan 1, 2024
@lolmaus
Copy link

lolmaus commented Jan 18, 2024

Ran into this issue too. Adding { recursive: true } as a second argument to readdirSync('.') does not change the return value (there are subfolders in the folder).

@strogonoff
Copy link
Contributor Author

strogonoff commented Jan 18, 2024

I use this for the time being, seems to work but obviously is suboptimal and inefficient:

const readdirRecursive = (
  /** Directory to list. */
  dir: string,
  /**
   * Directory to output paths relative to.
   * (Don’t specify, used for recursion.)
   */
  relativeTo?: string,
):
Effect.Effect<FileSystem.FileSystem, PlatformError, readonly string[]> =>
Effect.gen(function * (_) {
  const fs = yield * _(FileSystem.FileSystem);
  const dirEntries = yield * _(
    fs.readDirectory(dir),
    Effect.map(basenames => basenames.map(name => join(dir, name))),
  );

  const dirEntryStats:
  Record<string, FileSystem.File.Info> =
  yield * _(Effect.reduceEffect(
    dirEntries.map(path => pipe(
      fs.stat(path),
      Effect.map(stat => ({ [path]: stat })),
    )),
    Effect.succeed({}),
    (accum, item) => ({ ...accum, ...item }),
    { concurrency: 10 },
  ));

  const recursiveListings = dirEntries.map(path =>
    dirEntryStats[path]?.type === 'Directory'
      ? readdirRecursive(path, relativeTo ?? dir)
      : Effect.succeed([relative(relativeTo ?? dir, path)])
    );

  const entries = yield * _(
    Effect.all(recursiveListings, { concurrency: 10 }),
    Effect.map(resultLists => resultLists.flat()),
  );

  return entries;
});

@DadeSko DadeSko moved this to Created in Issues Management Jan 29, 2024
@mikearnaldi mikearnaldi moved this from Created to Active Discussion in Issues Management Feb 3, 2024
@BastLast
Copy link

got the same issue with node 18.15, updated to node 18.19 and it fixed the issue :)

You don't need to update to node 20 :) latest version of node 18 are fine.

@tim-smart tim-smart linked a pull request Apr 2, 2024 that will close this issue
@github-project-automation github-project-automation bot moved this from Active Discussion to Done in Issues Management Apr 2, 2024
debanjum added a commit to khoj-ai/khoj that referenced this issue Apr 9, 2024
- `fs.readdir' func in node version 18.18.2 has buggy `recursive' option
  See nodejs/node#48640, Effect-TS/effect#1801 for details

- We were recursing down a folder in two ways on the Desktop app.
  Remove `recursive: True' option to the `fs.readdirSync' method call
  to recurse down via app code only
debanjum added a commit to khoj-ai/khoj that referenced this issue Apr 9, 2024
- `fs.readdir' func in node version 18.18.2 has buggy `recursive' option
  See nodejs/node#48640, Effect-TS/effect#1801 for details

- We were recursing down a folder in two ways on the Desktop app.
  Remove `recursive: True' option to the `fs.readdirSync' method call
  to recurse down via app code only
debanjum added a commit to khoj-ai/khoj that referenced this issue Apr 9, 2024
- `fs.readdir' func in node version 18.18.2 has buggy `recursive' option
  See nodejs/node#48640, Effect-TS/effect#1801 for details

- We were recursing down a folder in two ways on the Desktop app.
  Remove `recursive: True' option to the `fs.readdirSync' method call
  to recurse down via app code only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working platform
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants