[Babel 8] Use more native fs methods #16459
Conversation
JLHwung
added
the
PR: Internal (next major) 🏠
| @@ -119,9 +119,7 @@ export default async function ({ | |||
|
|
|||
| const files = util.readdir(dirname, cliOptions.includeDotfiles); | |||
| for (const filename of files) { | |||
| const src = path.join(dirname, filename); | |||
There was a problem hiding this comment.
util.readdir now returns the joined path, so we don't have to join it here.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56803 |
| }, | ||
| // @ts-expect-error improve @types/fs-readdir-recursive typings | ||
| [], | ||
| dirname, |
There was a problem hiding this comment.
Use dirname as the prefix argument so that readdirRecursive will return the joined path without extra loop or .map. Unfortunately the typing here
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d152bf80d3c7df9104dbac9e369cdd64110601a7/types/fs-readdir-recursive/index.d.ts#L1-L4
only documented the first two parameters.
There was a problem hiding this comment.
DefinitelyTyped PR: DefinitelyTyped/DefinitelyTyped#69489
| // readdirSyncRecursive conducts BFS, sort the entries so we can match the DFS behaviour of fs-readdir-recursive | ||
| // https://github.com/nodejs/node/blob/d6b12f5b77e35c58a611d614cf0aac674ec2c3ed/lib/fs.js#L1421 |
There was a problem hiding this comment.
Q: does this matter in practice?
There was a problem hiding this comment.
The ordering are different as files in sub folders will be visited after files in top level folders, see #16459 (comment)
| (!filter || filter(filename)) | ||
| ); | ||
| }) | ||
| .map(dirent => path.join(dirent.path, dirent.name)) |
There was a problem hiding this comment.
The Dirent#path has been deprecated in favour of Dirent#parentPath, however to use parentPath we will have to further bump Node.js requirements to ^18.20.0 || ^20.12.0 || >= 21.4.0. I would like to hear about your opinions.
There was a problem hiding this comment.
OK, I will open a new PR to bump node and replace Dirent#path usage once the node typings are ready: DefinitelyTyped/DefinitelyTyped#69490
| .map(dirent => path.join(dirent.path, dirent.name)) | ||
| // readdirSyncRecursive conducts BFS, sort the entries so we can match the DFS behaviour of fs-readdir-recursive | ||
| // https://github.com/nodejs/node/blob/d6b12f5b77e35c58a611d614cf0aac674ec2c3ed/lib/fs.js#L1421 | ||
| .sort(alphasort) |
There was a problem hiding this comment.
Babel cli does not rely on the filenames ordering. The sort calls is meant to preserve the test log order.
If we want to change the ordering in Babel 8, we can remove this and the glob.sync usage in options.ts.
In this PR we use the following fs native methods available on Node.js 18+ for Babel 8.
fs.mkdirSync(_, { recursive: true }). The polyfill is already available, in this PR we replaced moremake-dirusagefs.rmSync(_, { force: true, recursive: true }), a polyfill is also added.fs.readdirSync(_, { recursive: true, withFileTypes: true })