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

[Babel 8] Use more native fs methods #16459

Merged
merged 5 commits into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions babel.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,44 @@ function pluginPolyfillsOldNode({ template, types: t }) {
// https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V16.md#v8-93
replacement: template`hasOwnProperty.call`,
},
{
name: "fs.rmSync",
necessary({ node, parent }) {
return (
t.isCallExpression(parent, { callee: node }) &&
parent.arguments.length > 1
);
},
supported({ parent: { arguments: args } }) {
return (
t.isObjectExpression(args[1]) &&
args[1].properties.length === 2 &&
t.isIdentifier(args[1].properties[0].key, { name: "force" }) &&
t.isBooleanLiteral(args[1].properties[0].value, { value: true }) &&
t.isIdentifier(args[1].properties[1].key, { name: "recursive" }) &&
t.isBooleanLiteral(args[1].properties[1].value, { value: true })
);
},
// fs.rmSync has been introduced in Node.js 14.14
// https://nodejs.org/api/fs.html#fsrmsyncpath-options
replacement: template`
((v,w)=>(v=v.split("."),w=w.split("."),+v[0]>+w[0]||v[0]==w[0]&&+v[1]>=+w[1]))(process.versions.node, "14.14")
? fs.rmSync
: function d(/* path */ p) {
if (fs.existsSync(p)) {
fs.readdirSync(p).forEach(function (f) {
const /* currentPath */ c = p + "/" + f;
if (fs.lstatSync(c).isDirectory()) {
d(c);
} else {
fs.unlinkSync(c);
}
});
fs.rmdirSync(p);
}
}
`,
},
];

return {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"@rollup/plugin-replace": "^5.0.5",
"@rollup/plugin-terser": "^0.4.4",
"@types/jest": "^29.5.11",
"@types/node": "^20.11.5",
"@types/node": "^20.12.7",
"@yarnpkg/types": "^4.0.0",
"babel-plugin-transform-charcodes": "^0.2.0",
"c8": "^8.0.1",
Expand Down
4 changes: 1 addition & 3 deletions packages/babel-cli/src/babel/dir.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

util.readdir now returns the joined path, so we don't have to join it here.


const written = await handleFile(src, dirname);
const written = await handleFile(filename, dirname);
if (written) count += 1;
}

Expand Down
12 changes: 4 additions & 8 deletions packages/babel-cli/src/babel/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,17 +158,13 @@ export default async function ({

const stat = fs.statSync(filename);
if (stat.isDirectory()) {
const dirname = filename;

util
.readdirForCompilable(
_filenames.push(
...util.readdirForCompilable(
filename,
cliOptions.includeDotfiles,
cliOptions.extensions,
)
.forEach(function (filename) {
_filenames.push(path.join(dirname, filename));
});
),
);
} else {
_filenames.push(filename);
}
Expand Down
5 changes: 2 additions & 3 deletions packages/babel-cli/src/babel/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import fs from "fs";
import commander from "commander";
import { version, DEFAULT_EXTENSIONS } from "@babel/core";
import * as glob from "glob";
import { alphasort } from "./util.ts";

import type { InputOptions } from "@babel/core";

Expand Down Expand Up @@ -216,9 +217,7 @@ export default function parseArgv(args: Array<string>): CmdOptions | null {
let files = process.env.BABEL_8_BREAKING
? // glob 9+ no longer sorts the result, here we maintain the glob 7 behaviour
// https://github.com/isaacs/node-glob/blob/c3cd57ae128faa0e9190492acc743bb779ac4054/common.js#L151
glob.sync(input, { dotRelative: true }).sort(function alphasort(a, b) {
return a.localeCompare(b, "en");
})
glob.sync(input, { dotRelative: true }).sort(alphasort)
: // @ts-expect-error When USE_ESM is true and BABEL_8_BREAKING is off,
// the glob package is an ESM wrapper of the CJS glob 7
(USE_ESM ? glob.default.sync : glob.sync)(input);
Expand Down
60 changes: 40 additions & 20 deletions packages/babel-cli/src/babel/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,54 @@ export function chmod(src: string, dest: string): void {
}
}

export function alphasort(a: string, b: string) {
return a.localeCompare(b, "en");
}

type ReaddirFilter = (filename: string) => boolean;

export function readdir(
dirname: string,
includeDotfiles: boolean,
filter?: ReaddirFilter,
): Array<string> {
return readdirRecursive(dirname, (filename, index, currentDirectory) => {
const stat = fs.statSync(path.join(currentDirectory, filename));

if (stat.isDirectory()) return true;

if (process.env.BABEL_8_BREAKING) {
return (
(includeDotfiles || filename[0] !== ".") && (!filter || filter(filename))
fs
.readdirSync(dirname, { recursive: true, withFileTypes: true })
.filter(dirent => {
// exclude directory entries from readdir results
if (dirent.isDirectory()) return false;
const filename = dirent.name;
return (
(includeDotfiles || filename[0] !== ".") &&
(!filter || filter(filename))
);
})
.map(dirent => path.join(dirent.path, dirent.name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This all seems ok to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will open a new PR to bump node and replace Dirent#path usage once the node typings are ready: DefinitelyTyped/DefinitelyTyped#69490

// 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
Comment on lines +43 to +44
Copy link
Member

Choose a reason for hiding this comment

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

Q: does this matter in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ordering are different as files in sub folders will be visited after files in top level folders, see #16459 (comment)

.sort(alphasort)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

);
});
} else {
return readdirRecursive(
"",
(filename, index, currentDirectory) => {
const stat = fs.statSync(path.join(currentDirectory, filename));

// ensure we recurse into .* folders
if (stat.isDirectory()) return true;

return (
(includeDotfiles || filename[0] !== ".") &&
(!filter || filter(filename))
);
},
// @ts-expect-error improve @types/fs-readdir-recursive typings
[],
dirname,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

);
}
}

export function readdirForCompilable(
Expand Down Expand Up @@ -109,19 +141,7 @@ export async function compile(filename: string, opts: InputOptions) {
}

export function deleteDir(path: string): void {
if (fs.existsSync(path)) {
fs.readdirSync(path).forEach(function (file) {
const curPath = path + "/" + file;
if (fs.lstatSync(curPath).isDirectory()) {
// recurse
deleteDir(curPath);
} else {
// delete file
fs.unlinkSync(curPath);
}
});
fs.rmdirSync(path);
}
fs.rmSync(path, { force: true, recursive: true });
}

process.on("uncaughtException", function (err) {
Expand Down
48 changes: 22 additions & 26 deletions packages/babel-helper-transform-fixture-test-runner/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ import { diff } from "jest-diff";
import type { ChildProcess } from "child_process";
import { spawn } from "child_process";
import os from "os";
import { sync as makeDir } from "make-dir";
import readdir from "fs-readdir-recursive";
import readdirRecursive from "fs-readdir-recursive";

import { createRequire } from "module";
const require = createRequire(import.meta.url);
Expand Down Expand Up @@ -664,39 +663,36 @@ const nodeGte8 = parseInt(process.versions.node, 10) >= 8;
// https://github.com/nodejs/node/issues/11422#issue-208189446
const tmpDir = realpathSync(os.tmpdir());

const readDir = function (loc: string, filter: Parameters<typeof readdir>[1]) {
const readDir = function (loc: string, pathFilter: (arg0: string) => boolean) {
const files: Record<string, string> = {};
if (fs.existsSync(loc)) {
readdir(loc, filter).forEach(function (filename) {
files[filename] = readFile(path.join(loc, filename));
});
if (process.env.BABEL_8_BREAKING) {
fs.readdirSync(loc, { withFileTypes: true, recursive: true })
.filter(dirent => dirent.isFile() && pathFilter(dirent.name))
.forEach(dirent => {
const fullpath = path.join(dirent.path, dirent.name);
files[path.relative(loc, fullpath)] = readFile(fullpath);
});
} else {
readdirRecursive(loc, pathFilter).forEach(function (filename) {
files[filename] = readFile(path.join(loc, filename));
});
}
}
return files;
};

const outputFileSync = function (filePath: string, data: string) {
makeDir(path.dirname(filePath));
fs.mkdirSync(path.dirname(filePath), { recursive: true });
fs.writeFileSync(filePath, data);
};

function deleteDir(path: string): void {
if (fs.existsSync(path)) {
fs.readdirSync(path).forEach(function (file) {
const curPath = path + "/" + file;
if (fs.lstatSync(curPath).isDirectory()) {
// recurse
deleteDir(curPath);
} else {
// delete file
fs.unlinkSync(curPath);
}
});
fs.rmdirSync(path);
}
fs.rmSync(path, { force: true, recursive: true });
}

const fileFilter = function (x: string) {
return x !== ".DS_Store";
const pathFilter = function (x: string) {
return path.basename(x) !== ".DS_Store";
};

const assertTest = function (
Expand Down Expand Up @@ -751,7 +747,7 @@ const assertTest = function (
}

if (opts.outFiles) {
const actualFiles = readDir(tmpDir, fileFilter);
const actualFiles = readDir(tmpDir, pathFilter);

Object.keys(actualFiles).forEach(function (filename) {
try {
Expand Down Expand Up @@ -865,8 +861,8 @@ export function buildProcessTests(
}

opts.testLoc = testLoc;
opts.outFiles = readDir(path.join(testLoc, "out-files"), fileFilter);
opts.inFiles = readDir(path.join(testLoc, "in-files"), fileFilter);
opts.outFiles = readDir(path.join(testLoc, "out-files"), pathFilter);
opts.inFiles = readDir(path.join(testLoc, "in-files"), pathFilter);

const babelrcLoc = path.join(testLoc, ".babelrc");
const babelIgnoreLoc = path.join(testLoc, ".babelignore");
Expand Down Expand Up @@ -903,7 +899,7 @@ export function buildProcessTests(
createHash("sha1").update(testLoc).digest("hex"),
);
deleteDir(tmpLoc);
makeDir(tmpLoc);
fs.mkdirSync(tmpLoc, { recursive: true });

const { inFiles } = opts;
for (const filename of Object.keys(inFiles)) {
Expand Down
10 changes: 5 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5496,12 +5496,12 @@ __metadata:
languageName: node
linkType: hard

"@types/node@npm:*, @types/node@npm:^20.11.5":
version: 20.11.5
resolution: "@types/node@npm:20.11.5"
"@types/node@npm:*, @types/node@npm:^20.12.7":
version: 20.12.7
resolution: "@types/node@npm:20.12.7"
dependencies:
undici-types: "npm:~5.26.4"
checksum: 10/9f31c471047d7b3e240ce7b77ff29b0d15e83be7e3feafb3d0b0d0931122b438b1eefa302a5a2e1e9849914ff3fd76aafbd8ccb372efb1331ba048da63bce6f8
checksum: 10/b4a28a3b593a9bdca5650880b6a9acef46911d58cf7cfa57268f048e9a7157a7c3196421b96cea576850ddb732e3b54bc982c8eb5e1e5ef0635d4424c2fce801
languageName: node
linkType: hard

Expand Down Expand Up @@ -7145,7 +7145,7 @@ __metadata:
"@rollup/plugin-replace": "npm:^5.0.5"
"@rollup/plugin-terser": "npm:^0.4.4"
"@types/jest": "npm:^29.5.11"
"@types/node": "npm:^20.11.5"
"@types/node": "npm:^20.12.7"
"@yarnpkg/types": "npm:^4.0.0"
babel-plugin-transform-charcodes: "npm:^0.2.0"
c8: "npm:^8.0.1"
Expand Down