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

fs#readdir(Sync) breaks when recursive + withFileTypes combined #48858

Closed
MattIPv4 opened this issue Jul 20, 2023 · 4 comments · Fixed by #48698
Closed

fs#readdir(Sync) breaks when recursive + withFileTypes combined #48858

MattIPv4 opened this issue Jul 20, 2023 · 4 comments · Fixed by #48698
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@MattIPv4
Copy link
Member

MattIPv4 commented Jul 20, 2023

Version

18.17.0

Platform

Darwin [snip] 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

touch a b c d e f

ls
a b c d e f

node
Welcome to Node.js v18.17.0.
Type ".help" for more information.
> const { readdir, readdirSync } = require('fs');
undefined
> readdir('./', console.log);
undefined
> null [ 'a', 'b', 'c', 'd', 'e', 'f' ]

> readdir('./', { withFileTypes: true }, console.log);
undefined
> null [
  Dirent { name: 'a', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'b', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'c', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'd', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'e', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'f', path: './', [Symbol(type)]: 1 }
]

> readdir('./', { recursive: true }, console.log);
null [ 'a', 'b', 'c', 'd', 'e', 'f' ]
undefined
> 
> readdir('./', { recursive: true, withFileTypes: true }, console.log);
null [
  Dirent { name: 'a', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'b', path: './', [Symbol(type)]: 1 }
]
undefined
> 
> readdirSync('./', { recursive: true });
[ 'a', 'b', 'c', 'd', 'e', 'f' ]
> readdirSync('./', { withFileTypes: true });
[
  Dirent { name: 'a', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'b', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'c', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'd', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'e', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'f', path: './', [Symbol(type)]: 1 }
]
> readdirSync('./', { recursive: true, withFileTypes: true });
[
  Dirent { name: 'a', path: './', [Symbol(type)]: 1 },
  Dirent { name: 'b', path: './', [Symbol(type)]: 1 }
]
>

How often does it reproduce? Is there a required condition?

Seemingly always

What is the expected behavior? Why is that the expected behavior?

Should return all the files

What do you see instead?

See the above reproduction. Even with no nested subdirectories, readdir / readdirSync fail to report all files in the current directory when the recursive option is supplied at the same time as the withFileTypes option.

Additional information

It looks like #48640 discusses this in the comments, and #48698 fixes this, but I did not see an actual issue tracking this broken behavior itself, and that it was shipped in 18.17.0

@agarzola
Copy link

I just ran into this on Node v20.5.0.

@That-Guy977
Copy link

This is also an issue with the callback fs.readdir as noted in the original issue

@VoltrexKeyva VoltrexKeyva added the fs Issues and PRs related to the fs subsystem / file system. label Jul 26, 2023
@MattIPv4 MattIPv4 changed the title fs#readdirSync breaks when recursive + withFileTypes combined fs#readdir(Sync) breaks when recursive + withFileTypes combined Jul 26, 2023
@MoLow MoLow linked a pull request Aug 10, 2023 that will close this issue
nodejs-github-bot pushed a commit that referenced this issue Aug 12, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@agarzola
Copy link

This is great news! Where can I find information about when we can expect v20.5.2 to be published (assuming this fix will be in that release)?

Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Aug 15, 2023
Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
rluvaton pushed a commit to rluvaton/node that referenced this issue Aug 15, 2023
Refs: nodejs#48640
PR-URL: nodejs#48698
Fixes: nodejs#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Aug 16, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Aug 17, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@Jieiku
Copy link

Jieiku commented Aug 28, 2023

workaround:

const fs = require('fs');

const path = './public/'
fs.readdirSync(path, { recursive: true, withFileTypes: false })
.forEach(
  (file) => {
    // check if file is directory, if not then log the path/file
    if (!fs.lstatSync(path+file).isDirectory()) {
      console.log(file);
    };
  },
);

targos pushed a commit that referenced this issue Oct 28, 2023
Refs: #48640
PR-URL: #48698
Fixes: #48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Refs: nodejs/node#48640
PR-URL: nodejs/node#48698
Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Refs: nodejs/node#48640
PR-URL: nodejs/node#48698
Fixes: nodejs/node#48858
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
cogwirrel added a commit to aws/aws-pdk that referenced this issue Oct 31, 2024
Node 18.17.x has a bug where `readdirSync` returns invalid entries when
both `recursive` and `withFileTypes` is specified:

nodejs/node#48858

We use our own implementation to recursively find template files to
ensure we work for older versions of node.
cogwirrel added a commit to aws/aws-pdk that referenced this issue Nov 1, 2024
Node 18.17.x has a bug where `readdirSync` returns invalid entries when
both `recursive` and `withFileTypes` is specified:

nodejs/node#48858

We use our own implementation to recursively find template files to
ensure we work for older versions of node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants