diff --git a/config.ts b/config.ts index 7c26d18..2a82774 100644 --- a/config.ts +++ b/config.ts @@ -5,7 +5,7 @@ import { type PromptEntry, promptSelect, } from "@std/cli/unstable-prompt-select"; -import { fromFileUrl, join, resolve } from "@std/path"; +import { dirname, join, resolve } from "@std/path"; import { parse as parseJSONC } from "@david/jsonc-morph"; import { resolve_config } from "./lib/rs_lib.js"; import { ValidationError } from "@cliffy/command"; @@ -182,15 +182,32 @@ export function actionHandler< ): (options: O, ...args: A) => Promise { return async function (this: unknown, context: O, ...args: A) { try { - const config = await readConfig( - rootPath?.(...args) ?? Deno.cwd(), + const root = rootPath?.(...args) ?? Deno.cwd(); + // Discover the config file only (cheap). The deploy file manifest is a + // full recursive walk of `root` that can be huge/slow, so it is collected + // lazily via `configContext.files` and paid for only by the upload path + // (see `readDeployFiles`). Read-only commands (whoami, orgs/apps/env list, + // logs, ...) never touch `.files`, so they never trigger the walk — which + // previously hung them when invoked from a large working directory. + const config = await discoverConfig( + root, context.config, - context.ignore ?? [], - context.allowNodeModules ?? false, - context.debug, ); + let collectedFiles: string[] | undefined; const configContext: ConfigContext = { ...getAppFromConfig(config), + get files(): string[] { + if (collectedFiles === undefined) { + collectedFiles = _internals.readDeployFiles( + root, + context.config, + context.ignore ?? [], + context.allowNodeModules ?? false, + context.debug, + ); + } + return collectedFiles; + }, configSaved: false, doNotCreate: false, save() { @@ -240,16 +257,71 @@ interface Config { path: string; content: string; }; - files: string[]; } -async function readConfig( +const CONFIG_FILE_NAMES = ["deno.json", "deno.jsonc"] as const; + +/** + * Locate the deploy config file (for `org`/`app`) WITHOUT collecting the deploy + * file manifest. This is intentionally cheap: an explicit `--config` is used + * as-is, otherwise we walk UP from the root looking for a `deno.json[c]`. It + * deliberately avoids `resolve_config`, whose recursive DOWNward file walk to + * build the upload manifest can be enormous — and is only needed by the upload + * path (`configContext.files` -> `readDeployFiles`), not by read-only commands. + */ +async function discoverConfig( + rootPath: string, + maybeConfigPath: string | undefined, +): Promise { + if (maybeConfigPath) { + const path = resolve(maybeConfigPath); + const content = await Deno.readTextFile(path); + return { config: { path, content } }; + } + + let dir = resolve(rootPath); + try { + if ((await Deno.stat(dir)).isFile) { + dir = dirname(dir); + } + } catch (err) { + // Non-existent root: nothing to discover. Any other error (e.g. permission) + // is real and must surface rather than be silently treated as "no config". + if (!(err instanceof Deno.errors.NotFound)) throw err; + } + + while (true) { + for (const name of CONFIG_FILE_NAMES) { + const path = join(dir, name); + try { + const content = await Deno.readTextFile(path); + return { config: { path, content } }; + } catch (err) { + // Not here; keep looking upward. Only a missing file means "not here" — + // a permission/IO error on an existing config must not be swallowed. + if (!(err instanceof Deno.errors.NotFound)) throw err; + } + } + const parent = dirname(dir); + if (parent === dir) { + return {}; + } + dir = parent; + } +} + +/** + * Collect the recursive deploy file manifest for the upload. This is the + * expensive full-directory walk (via the wasm `resolve_config`); call it only + * when the files are actually needed (i.e. from the upload path). + */ +function readDeployFiles( rootPath: string, maybeConfigPath: string | undefined, ignorePaths: string[], allowNodeModules: boolean, debug: boolean, -): Promise { +): string[] { // Only `--config ` is parsed as a config file; a positional root is not. const fromConfig = Boolean(maybeConfigPath); const config = resolve_config( @@ -259,19 +331,19 @@ async function readConfig( allowNodeModules, debug, ); - - if (config.path) { - const path = fromFileUrl(config.path); - const content = await Deno.readTextFile(path); - return { config: { path, content }, files: config.files }; - } - - return { files: config.files }; + return config.files; } +/** + * Test seam: `actionHandler`'s lazy `files` getter calls `readDeployFiles` + * through this indirection so tests can observe (or stub) the expensive walk and + * assert it runs only when `.files` is actually read. + */ +export const _internals = { readDeployFiles }; + function getAppFromConfig( configContent: Config, -): { org: undefined | string; app: undefined | string; files: string[] } { +): { org: undefined | string; app: undefined | string } { if (configContent.config) { const config = parseJSONC(configContent.config.content); const deployObj = config.asObject()?.getIfObject("deploy"); @@ -280,7 +352,6 @@ function getAppFromConfig( return { org: deployObj.get("org")?.value()?.asString(), app: deployObj.get("app")?.value()?.asString(), - files: configContent.files, }; } } @@ -288,7 +359,6 @@ function getAppFromConfig( return { org: undefined, app: undefined, - files: configContent.files, }; } diff --git a/tests/config.test.ts b/tests/config.test.ts index a68c78b..b5365d0 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,6 +1,6 @@ -import { assertEquals, assertStringIncludes } from "@std/assert"; +import { assert, assertEquals, assertStringIncludes } from "@std/assert"; import { join } from "@std/path"; -import { actionHandler } from "../config.ts"; +import { _internals, actionHandler } from "../config.ts"; import type { GlobalContext } from "../main.ts"; // Runs the deploy action handler on a temporary config @@ -94,3 +94,75 @@ Deno.test("deploy preserves comments and formatting (jsonc)", async () => { assertStringIncludes(out, "// keep this comment"); assertStringIncludes(out, "// and this one"); }); + +// Regression: a read-only command (one that never touches `config.files`) must +// NOT trigger the recursive deploy-manifest file walk. Before the lazy-`files` +// fix, `actionHandler` eagerly walked the whole working directory, so commands +// like `whoami`/`orgs list` hung for a very long time when invoked from a large +// directory tree. Rather than measure wall-clock time (flaky across machines), +// count invocations of the walk directly: it must run 0 times for a read-only +// action and exactly once when `.files` is read, while still yielding the real +// manifest on demand. +Deno.test("read-only action does not walk the file tree; .files stays lazy", async () => { + const dir = await Deno.makeTempDir(); + const originalReadDeployFiles = _internals.readDeployFiles; + let walkCount = 0; + _internals.readDeployFiles = (...args) => { + walkCount++; + return originalReadDeployFiles(...args); + }; + try { + await Deno.writeTextFile( + join(dir, "deno.json"), + `{ "deploy": { "org": "o", "app": "a" } }\n`, + ); + const encoder = new TextEncoder(); + const sub = join(dir, "src"); + await Deno.mkdir(sub, { recursive: true }); + for (let i = 0; i < 5; i++) { + await Deno.writeFile( + join(sub, `f${i}.ts`), + encoder.encode("export const x = 1;\n"), + ); + } + + const context = { + config: undefined, + ignore: [], + allowNodeModules: false, + debug: false, + } as unknown as GlobalContext; + + // Read-only action: reads org/app, never touches `.files`. + let seenOrg: string | undefined; + await actionHandler((config) => { + config.noCreate(); + config.noSave(); + seenOrg = config.org; + }, () => dir)(context); + + assertEquals(seenOrg, "o"); + // The whole point of the fix: not touching `.files` must skip the walk. + assertEquals(walkCount, 0, "read-only action must not walk the file tree"); + + // Same action, but this one reads `.files`, forcing the recursive walk. + let files: string[] = []; + await actionHandler((config) => { + config.noCreate(); + config.noSave(); + // Read twice to confirm the getter memoizes (a single walk). + files = config.files; + files = config.files; + }, () => dir)(context); + + // The walk runs exactly once, and the manifest is available on demand. + assertEquals(walkCount, 1, "reading .files must walk exactly once"); + assert( + files.length >= 5, + `expected the lazy manifest to include the tree files; got ${files.length}`, + ); + } finally { + _internals.readDeployFiles = originalReadDeployFiles; + await Deno.remove(dir, { recursive: true }); + } +});