From 23cf55783677d4937ab29527889a61eadfbe0eab Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Sun, 14 Jul 2024 16:20:49 -0700 Subject: [PATCH] Swap import-meta-resolve for plain filesystem walking (#131) --- .changeset/mean-ants-protect.md | 6 ++++ .github/renovate.json | 4 --- .ncurc.cjs | 1 - package-lock.json | 15 --------- package.json | 1 - src/__tests__/cli/index.test.ts | 11 ------- src/__tests__/cli/reexec.test.ts | 56 -------------------------------- src/cli/index.ts | 2 +- src/cli/reexec.ts | 51 +++++++++++++++++++++-------- src/cli/utils.ts | 15 +++------ 10 files changed, 50 insertions(+), 112 deletions(-) create mode 100644 .changeset/mean-ants-protect.md delete mode 100644 src/__tests__/cli/reexec.test.ts diff --git a/.changeset/mean-ants-protect.md b/.changeset/mean-ants-protect.md new file mode 100644 index 0000000..f89e604 --- /dev/null +++ b/.changeset/mean-ants-protect.md @@ -0,0 +1,6 @@ +--- +"hereby": minor +--- + +Swap import-meta-resolve for plain filesystem walking; this makes startup +roughly 10-20% faster and prevents a deprecation warning in Node 22+ diff --git a/.github/renovate.json b/.github/renovate.json index e566ba9..334443a 100644 --- a/.github/renovate.json +++ b/.github/renovate.json @@ -13,10 +13,6 @@ "matchDepTypes": ["engines"], "enabled": false }, - { - "matchPackageNames": ["import-meta-resolve"], - "allowedVersions": "^2" - }, { "matchPackageNames": ["npm"], "allowedVersions": "^8" diff --git a/.ncurc.cjs b/.ncurc.cjs index f9c112e..bcd65ce 100644 --- a/.ncurc.cjs +++ b/.ncurc.cjs @@ -4,7 +4,6 @@ module.exports = { if (dependencyName === "@ava/typescript") return "minor"; // Trying to support Node 12. if (dependencyName === "execa") return "patch"; // Trying to support Node 12. if (dependencyName === "command-line-usage") return "minor"; // Package got big. - if (dependencyName === "import-meta-resolve") return "minor"; // For old versions of node. if (dependencyName === "pretty-ms") return "minor"; // For old versions of node. if (dependencyName === "eslint") return "minor"; if (dependencyName === "eslint-plugin-ava") return "minor"; diff --git a/package-lock.json b/package-lock.json index 66e1556..662884a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,7 +11,6 @@ "dependencies": { "command-line-usage": "^6.1.3", "fastest-levenshtein": "^1.0.16", - "import-meta-resolve": "^2.2.2", "minimist": "^1.2.8", "picocolors": "^1.0.1", "pretty-ms": "^8.0.0" @@ -3515,15 +3514,6 @@ "node": ">=4" } }, - "node_modules/import-meta-resolve": { - "version": "2.2.2", - "resolved": "https://registry.npmjs.org/import-meta-resolve/-/import-meta-resolve-2.2.2.tgz", - "integrity": "sha512-f8KcQ1D80V7RnqVm+/lirO9zkOxjGxhaTC1IPrBGd3MEfNgmNG67tSUO9gTi2F3Blr2Az6g1vocaxzkVnWl9MA==", - "funding": { - "type": "github", - "url": "https://github.com/sponsors/wooorm" - } - }, "node_modules/import-modules": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/import-modules/-/import-modules-2.1.0.tgz", @@ -8755,11 +8745,6 @@ } } }, - "import-meta-resolve": { - "version": "2.2.2", - "resolved": "https://registry.npmjs.org/import-meta-resolve/-/import-meta-resolve-2.2.2.tgz", - "integrity": "sha512-f8KcQ1D80V7RnqVm+/lirO9zkOxjGxhaTC1IPrBGd3MEfNgmNG67tSUO9gTi2F3Blr2Az6g1vocaxzkVnWl9MA==" - }, "import-modules": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/import-modules/-/import-modules-2.1.0.tgz", diff --git a/package.json b/package.json index c742742..f5d20ce 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,6 @@ "dependencies": { "command-line-usage": "^6.1.3", "fastest-levenshtein": "^1.0.16", - "import-meta-resolve": "^2.2.2", "minimist": "^1.2.8", "picocolors": "^1.0.1", "pretty-ms": "^8.0.0" diff --git a/src/__tests__/cli/index.test.ts b/src/__tests__/cli/index.test.ts index 01b8e7f..96b6445 100644 --- a/src/__tests__/cli/index.test.ts +++ b/src/__tests__/cli/index.test.ts @@ -2,7 +2,6 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import test from "ava"; -import { resolve } from "import-meta-resolve"; import { main, selectTasks } from "../../cli/index.js"; import { loadHerebyfile } from "../../cli/loadHerebyfile.js"; @@ -111,8 +110,6 @@ test("main print tasks", async (t) => { .returns(() => fixturesPath) .setup((d) => d.log) .returns((message) => log.push(["log", message.replace(/\r/g, "")])) - .setup((d) => d.resolve) - .returns(resolve) .setup((d) => d.chdir) .returns((directory) => t.is(directory, fixturesPath)); @@ -133,8 +130,6 @@ test("main success", async (t) => { .returns(() => fixturesPath) .setup((d) => d.log) .returns((message) => log.push(["log", message.replace(/\r/g, "")])) - .setup((d) => d.resolve) - .returns(resolve) .setup((d) => d.chdir) .returns((directory) => t.is(directory, fixturesPath)) .setup((d) => d.simplifyPath) @@ -159,8 +154,6 @@ test("main failure", async (t) => { .returns(() => fixturesPath) .setup((d) => d.log) .returns((message) => log.push(["log", message.replace(/\r/g, "")])) - .setup((d) => d.resolve) - .returns(resolve) .setup((d) => d.chdir) .returns((directory) => t.is(directory, fixturesPath)) .setup((d) => d.simplifyPath) @@ -193,8 +186,6 @@ test("main user error", async (t) => { .returns(() => fixturesPath) .setup((d) => d.log) .returns((message) => log.push(["log", message.replace(/\r/g, "")])) - .setup((d) => d.resolve) - .returns(resolve) .setup((d) => d.chdir) .returns((directory) => t.is(directory, fixturesPath)) .setup((d) => d.simplifyPath) @@ -293,8 +284,6 @@ test("main print version", async (t) => { .returns(() => fixturesPath) .setup((d) => d.log) .returns((message) => t.is(message, `hereby ${version}`)) - .setup((d) => d.resolve) - .returns(resolve) .setup((d) => d.chdir) .returns((directory) => t.is(directory, fixturesPath)); diff --git a/src/__tests__/cli/reexec.test.ts b/src/__tests__/cli/reexec.test.ts deleted file mode 100644 index 61baef4..0000000 --- a/src/__tests__/cli/reexec.test.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { fileURLToPath } from "node:url"; - -import test from "ava"; - -import { reexec, type ReExecD } from "../../cli/reexec.js"; -import { UserError } from "../../cli/utils.js"; -import { mock } from "../__helpers__/index.js"; - -const cliIndexURL = new URL("../../cli/index.js", import.meta.url).toString(); -const herebyfilePath = fileURLToPath(new URL("__fixtures__/Herebyfile.mjs", import.meta.url)); - -test("no re-exec", async (t) => { - let callCount = 0; - - const dMock = mock(t) - .setup((d) => d.resolve) - .returns(async () => { - callCount++; - switch (callCount) { - case 1: - return cliIndexURL; - case 2: - return cliIndexURL; - default: - t.fail(); - throw new Error("too many calls"); - } - }); - - const returnNow = await reexec(dMock.object(), herebyfilePath); - t.false(returnNow); -}); - -test("fail to resolve other", async (t) => { - let callCount = 0; - - const dMock = mock(t) - .setup((d) => d.resolve) - .returns(async () => { - callCount++; - switch (callCount) { - case 1: - return cliIndexURL; - case 2: - throw new Error("Cannot find package 'hereby' imported from ..."); - default: - t.fail(); - throw new Error("too many calls"); - } - }); - - await t.throwsAsync(async () => await reexec(dMock.object(), herebyfilePath), { - instanceOf: UserError, - message: "Unable to find hereby; ensure hereby is installed in your package.", - }); -}); diff --git a/src/cli/index.ts b/src/cli/index.ts index c6f21fe..9640210 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -38,7 +38,7 @@ async function mainWorker(d: D) { let herebyfilePath = args.herebyfile ?? findHerebyfile(d.cwd()); herebyfilePath = path.resolve(d.cwd(), herebyfilePath); - if (await reexec(d, herebyfilePath)) { + if (await reexec(herebyfilePath)) { return; } diff --git a/src/cli/reexec.ts b/src/cli/reexec.ts index 648cae6..094a4d6 100644 --- a/src/cli/reexec.ts +++ b/src/cli/reexec.ts @@ -1,17 +1,19 @@ -import { pathToFileURL } from "node:url"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; -import { type D, UserError } from "./utils.js"; +import { findUp, UserError } from "./utils.js"; -export type ReExecD = Pick; - -const cliExportName = "hereby/cli"; +const thisCLI = fileURLToPath(new URL("../cli.js", import.meta.url)); +const distCLIPath = path.join("dist", "cli.js"); +const expectedCLIPath = path.join("node_modules", "hereby", distCLIPath); /** * Checks to see if we need to re-exec another version of hereby. * If this function returns true, the caller should return immediately * and do no further work. */ -export async function reexec(d: ReExecD, herebyfilePath: string): Promise { +export async function reexec(herebyfilePath: string): Promise { // If hereby is installed globally, but run against a Herebyfile in some // other package, that Herebyfile's import will resolve to a different // installation of the hereby package. There's no guarantee that the two @@ -20,20 +22,43 @@ export async function reexec(d: ReExecD, herebyfilePath: string): Promise { + const p = path.resolve(dir, expectedCLIPath); + // This is the typical case; we've walked up and found it in node_modules. + if (fs.existsSync(p)) return p; - const thisCLI = await d.resolve(cliExportName, import.meta.url); - let otherCLI: string; + // Otherwise, we check to see if we're self-resolving. Realistically, + // this only happens when developing hereby itself. + // + // Technically, this should go before the above check since self-resolution + // comes before node_modules resolution, but this could only happen if hereby + // happened to depend on itself somehow. + const packageJsonPath = path.join(dir, "package.json"); + if (fs.existsSync(packageJsonPath)) { + const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, "utf8")); + if (packageJson.name === "hereby") { + return path.resolve(dir, distCLIPath); + } + } + return undefined; + }); - try { - otherCLI = await d.resolve(cliExportName, pathToFileURL(herebyfilePath).toString()); - } catch { + if (!otherCLI) { throw new UserError("Unable to find hereby; ensure hereby is installed in your package."); } - if (thisCLI === otherCLI) { + if (fs.realpathSync(thisCLI) === fs.realpathSync(otherCLI)) { return false; } - await import(otherCLI); + // Note: calling pathToFileURL is required on Windows to disambiguate URLs + // from drive letters. + await import(pathToFileURL(otherCLI).toString()); return true; } diff --git a/src/cli/utils.ts b/src/cli/utils.ts index 7fdcdae..7d5327f 100644 --- a/src/cli/utils.ts +++ b/src/cli/utils.ts @@ -24,14 +24,14 @@ export function simplifyPath(p: string) { return p; } -export function findUp(p: string, predicate: (dir: string) => T | undefined): T | undefined { - const root = path.parse(p).root; +export function findUp(dir: string, predicate: (dir: string) => T | undefined): T | undefined { + const root = path.parse(dir).root; while (true) { - const result = predicate(p); + const result = predicate(dir); if (result !== undefined) return result; - if (p === root) break; - p = path.dirname(p); + if (dir === root) break; + dir = path.dirname(dir); } return undefined; @@ -56,7 +56,6 @@ export interface D { readonly version: () => string; // Third-party package imports. - readonly resolve: (specifier: string, parent: string) => Promise; readonly prettyMilliseconds: (milliseconds: number) => string; } @@ -81,10 +80,6 @@ export async function real(): Promise { const packageJson = fs.readFileSync(packageJsonURL, "utf8"); return JSON.parse(packageJson).version; }, - resolve: async (specifier, parent) => { - const { resolve } = await import("import-meta-resolve"); - return resolve(specifier, parent); - }, prettyMilliseconds, }; /* eslint-enable no-restricted-globals */