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

fix: don't include filename in path when calling readdir with withFileTypes: true #1024

Merged
merged 5 commits into from
Apr 14, 2024

Conversation

jonsch318
Copy link
Contributor

@jonsch318 jonsch318 commented Apr 10, 2024

This solves issue #1007 and is in large part the solution of @zzzzBuv though with some tests changed for conformance with node and readdir recursive fix.

src/dirent.path is only the path of the parent dir and dirent.name is either the filename+extension or the directory name. This is now reflected in readdir. Because readdir constructs the filenames from the readdir we join(path,name).

I had to change the test with readdir({withFileType: true}) because they were out of spec with node.

To test for conformance i used a simple script (node version v21.7.2)

const fs = require('fs');
const path = require('path');

async function Test() {
  const dir = path.join(process.cwd());

  console.log('readdirSync(withFileTypes, recursive)');

  let res = fs.readdirSync(dir, { withFileTypes: true, recursive: true });
  console.log(res);

  console.log('promise readdir(withFileTypes, recursive)');

  res = await fs.promises.readdir(dir, { withFileTypes: true, recursive: true });
  console.log(res);

  console.log('promise readdir(recursive)');

  res = fs.readdirSync(dir, { withFileTypes: false, recursive: true });
  console.log(res);
}

Test();

in the following file structure
image

it results in the following log:

readdirSync(withFileTypes, recursive)
[
  Dirent {
    name: 'a',
    parentPath: '/home/jonas/src/temp/readdir-test',
    path: '/home/jonas/src/temp/readdir-test',
    [Symbol(type)]: 2
  },
  Dirent {
    name: 'rf1',
    parentPath: '/home/jonas/src/temp/readdir-test',
    path: '/home/jonas/src/temp/readdir-test',
    [Symbol(type)]: 1
  },
  Dirent {
    name: 'test.js',
    parentPath: '/home/jonas/src/temp/readdir-test',
    path: '/home/jonas/src/temp/readdir-test',
    [Symbol(type)]: 1
  },
  Dirent {
    name: '.af2',
    parentPath: '/home/jonas/src/temp/readdir-test/a',
    path: '/home/jonas/src/temp/readdir-test/a',
    [Symbol(type)]: 1
  },
  Dirent {
    name: 'af1',
    parentPath: '/home/jonas/src/temp/readdir-test/a',
    path: '/home/jonas/src/temp/readdir-test/a',
    [Symbol(type)]: 1
  }
]
promise readdir(withFileTypes, recursive)
[
  Dirent {
    name: 'a',
    parentPath: '/home/jonas/src/temp/readdir-test',
    path: '/home/jonas/src/temp/readdir-test',
    [Symbol(type)]: 2
  },
  Dirent {
    name: 'rf1',
    parentPath: '/home/jonas/src/temp/readdir-test',
    path: '/home/jonas/src/temp/readdir-test',
    [Symbol(type)]: 1
  },
  Dirent {
    name: 'test.js',
    parentPath: '/home/jonas/src/temp/readdir-test',
    path: '/home/jonas/src/temp/readdir-test',
    [Symbol(type)]: 1
  },
  Dirent {
    name: '.af2',
    parentPath: '/home/jonas/src/temp/readdir-test/a',
    path: '/home/jonas/src/temp/readdir-test/a',
    [Symbol(type)]: 1
  },
  Dirent {
    name: 'af1',
    parentPath: '/home/jonas/src/temp/readdir-test/a',
    path: '/home/jonas/src/temp/readdir-test/a',
    [Symbol(type)]: 1
  }
]
promise readdir(recursive)
[ 'a', 'rf1', 'test.js', 'a/.af2', 'a/af1' ]

as i can see the test reflect that.

Resolves #1007

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 10, 2024

Looks like a good start, just needs the Windows tests fixed

@jonsch318
Copy link
Contributor Author

jonsch318 commented Apr 10, 2024

I removed the overly long commit description from the one commit :)
and hopefully fixed the windows bug. i cant test that locally

@jonsch318
Copy link
Contributor Author

@G-Rath my bad i forgot that string replace isn`t inplace. I have switched to my windows machine and this passes all tests so it should now work.

@jonsch318
Copy link
Contributor Author

okay last try... sorry for the careless editing. Now everything should be fine

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 14, 2024

Whoops didn't see there were new pushes - no worries about the repeated failing of CI.

I'm happy to keep approving the workflow runs until it passes 😛

@G-Rath G-Rath changed the title fix: readdir withFileType path bug fix: don't include file name in path when calling readdir with withFileTypes: true Apr 14, 2024
@G-Rath G-Rath changed the title fix: don't include file name in path when calling readdir with withFileTypes: true fix: don't include filename in path when calling readdir with withFileTypes: true Apr 14, 2024
@G-Rath G-Rath merged commit 711c4bd into streamich:master Apr 14, 2024
10 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 14, 2024
## [4.8.2](v4.8.1...v4.8.2) (2024-04-14)

### Bug Fixes

* don't include filename in `path` when calling `readdir` with `withFileTypes: true` ([#1024](#1024)) ([711c4bd](711c4bd))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

readdir with withFileTypes:true incorrectly includes file name in path
2 participants