Skip to content

Commit

Permalink
Swap import-meta-resolve for plain filesystem walking (#131)
Browse files Browse the repository at this point in the history
  • Loading branch information
jakebailey authored Jul 14, 2024
1 parent 6d277a7 commit 23cf557
Show file tree
Hide file tree
Showing 10 changed files with 50 additions and 112 deletions.
6 changes: 6 additions & 0 deletions .changeset/mean-ants-protect.md
Original file line number Diff line number Diff line change
@@ -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+
4 changes: 0 additions & 4 deletions .github/renovate.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@
"matchDepTypes": ["engines"],
"enabled": false
},
{
"matchPackageNames": ["import-meta-resolve"],
"allowedVersions": "^2"
},
{
"matchPackageNames": ["npm"],
"allowedVersions": "^8"
Expand Down
1 change: 0 additions & 1 deletion .ncurc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
15 changes: 0 additions & 15 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
11 changes: 0 additions & 11 deletions src/__tests__/cli/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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));

Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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));

Expand Down
56 changes: 0 additions & 56 deletions src/__tests__/cli/reexec.test.ts

This file was deleted.

2 changes: 1 addition & 1 deletion src/cli/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
51 changes: 38 additions & 13 deletions src/cli/reexec.ts
Original file line number Diff line number Diff line change
@@ -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<D, "error" | "resolve">;

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<boolean> {
export async function reexec(herebyfilePath: string): Promise<boolean> {
// 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
Expand All @@ -20,20 +22,43 @@ export async function reexec(d: ReExecD, herebyfilePath: string): Promise<boolea
// Rather than trying to fix this by messing around with Node's resolution
// (which won't work in ESM anyway), instead opt to figure out the location
// of hereby as imported by the Herebyfile, and then "reexec" it by importing.
//
// This code used to use `import.meta.resolve` to find `hereby/cli`, but
// manually encoding this behavior is faster and avoids the dependency.
// If Node ever makes the two-argument form of `import.meta.resolve` unflagged,
// we could switch to that.

const otherCLI = findUp(path.dirname(herebyfilePath), (dir) => {
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;
}
15 changes: 5 additions & 10 deletions src/cli/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ export function simplifyPath(p: string) {
return p;
}

export function findUp<T extends {}>(p: string, predicate: (dir: string) => T | undefined): T | undefined {
const root = path.parse(p).root;
export function findUp<T extends {}>(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;
Expand All @@ -56,7 +56,6 @@ export interface D {
readonly version: () => string;

// Third-party package imports.
readonly resolve: (specifier: string, parent: string) => Promise<string>;
readonly prettyMilliseconds: (milliseconds: number) => string;
}

Expand All @@ -81,10 +80,6 @@ export async function real(): Promise<D> {
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 */
Expand Down

0 comments on commit 23cf557

Please sign in to comment.