From 481d08b49cf3b83b742dc37c470cf6a8eedccdca Mon Sep 17 00:00:00 2001 From: UtkarshBhardwaj007 Date: Wed, 20 May 2026 10:38:06 +0100 Subject: [PATCH] fix(dot deploy): always install before build, surface build errors clearly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `detectInstallConfig` previously short-circuited when a `node_modules/` directory existed at the project root. A stale `node_modules/` left over from a branch switch or lockfile bump bypassed that guard and let Vite build against package versions that didn't match the lockfile, producing opaque "X is not exported by Y" errors. Install unconditionally so pnpm/yarn/npm can reconcile (idempotent ~1s no-op when in sync). Drops the now-dead `hasNodeModules` field from `DetectInput`. Also fixes two CLI bugs that compounded the problem when a build did fail: - `runStreamed` only kept the last 10 lines of build output in the rejection message. Vite/Rollup print the actual error followed by a 10–20 line stack trace, so the slice consistently dropped the message and showed only the trace. Widen the snippet to the full 40-line tail buffer. - `DeployScreen`'s frontend / contracts / playground rows each set `value={state.error}` while the screen-level "deploy failed" row at the bottom rendered the same message a second time. Worse, the frontend's `upload + dotns` row showed the error even when it was the `build` step that failed. Drop the in-section error text so the `✕` mark indicates which step failed and the bottom row carries the message once. Adds a Vite-shaped regression test to `process.test.ts` ensuring an "error message + long stack trace" output preserves the message end-to-end. --- .changeset/always-install-before-build.md | 7 ++++ src/commands/deploy/DeployScreen.tsx | 3 -- src/utils/build/detect.test.ts | 18 ++++----- src/utils/build/detect.ts | 20 ++++----- src/utils/build/runner.ts | 1 - src/utils/deploy/run.test.ts | 2 +- src/utils/process.test.ts | 49 +++++++++++++++++++---- src/utils/process.ts | 10 +++-- 8 files changed, 75 insertions(+), 35 deletions(-) create mode 100644 .changeset/always-install-before-build.md diff --git a/.changeset/always-install-before-build.md b/.changeset/always-install-before-build.md new file mode 100644 index 0000000..ba39aa8 --- /dev/null +++ b/.changeset/always-install-before-build.md @@ -0,0 +1,7 @@ +--- +"playground-cli": patch +--- + +`dot deploy` and `dot build` now run `pnpm install` (or the project's package manager equivalent) before every build, not just when `node_modules/` is missing. A stale `node_modules/` left over from a branch switch or a lockfile bump used to slip past the missing-folder guard and produce opaque Vite/Rollup errors like `"X is not exported by ..."`; the only fix was to re-run `pnpm install` by hand. The install step is idempotent (~1s when nothing has changed), so the happy path is essentially unaffected. + +Also surfaces more of the failing build's output in the CLI error message (40 lines instead of 10), so when a build does fail the actual error line — not just the trailing stack trace — makes it into the rendered output. And the same error no longer renders twice in the deploy TUI: the per-section row marks which step failed with `✕`, and the bottom `deploy failed` row carries the message once. diff --git a/src/commands/deploy/DeployScreen.tsx b/src/commands/deploy/DeployScreen.tsx index 367bf33..684529f 100644 --- a/src/commands/deploy/DeployScreen.tsx +++ b/src/commands/deploy/DeployScreen.tsx @@ -997,7 +997,6 @@ function RunningStage({ @@ -1030,7 +1029,6 @@ function ContractsSectionView({ state }: { state: ContractsSectionState }) { {state.contracts.length > 0 && ( @@ -1072,7 +1070,6 @@ function FrontendSectionView({ state }: { state: FrontendSectionState }) { {running && state.latestLog && {truncate(state.latestLog, 120)}} diff --git a/src/utils/build/detect.test.ts b/src/utils/build/detect.test.ts index a68c0c9..5e08969 100644 --- a/src/utils/build/detect.test.ts +++ b/src/utils/build/detect.test.ts @@ -28,7 +28,6 @@ function input(overrides: Partial = {}): DetectInput { packageJson: null, lockfiles: new Set(), configFiles: new Set(), - hasNodeModules: true, cargoToml: null, ...overrides, }; @@ -147,19 +146,23 @@ describe("detectBuildConfig", () => { }); describe("detectInstallConfig", () => { - it("returns null when node_modules is already present", () => { + it("returns an install command even when node_modules already exists (idempotent reconcile)", () => { + // A stale node_modules/ — e.g. after a branch switch — bypasses any + // missing-folder guard and lets the build run against package versions + // that don't match the lockfile. We install unconditionally so that + // case can't reach the build step. expect( detectInstallConfig( input({ packageJson: { dependencies: { vite: "^5.0.0" } }, - hasNodeModules: true, + lockfiles: new Set(["pnpm-lock.yaml"]), }), ), - ).toBeNull(); + ).toEqual({ cmd: "pnpm", args: ["install"], description: "pnpm install" }); }); it("returns null when package.json is missing", () => { - expect(detectInstallConfig(input({ hasNodeModules: false }))).toBeNull(); + expect(detectInstallConfig(input())).toBeNull(); }); it("returns null when the project declares no dependencies", () => { @@ -169,7 +172,6 @@ describe("detectInstallConfig", () => { detectInstallConfig( input({ packageJson: { scripts: { build: "echo hi" } }, - hasNodeModules: false, }), ), ).toBeNull(); @@ -180,7 +182,6 @@ describe("detectInstallConfig", () => { detectInstallConfig( input({ packageJson: { devDependencies: { vite: "^7.0.0" } }, - hasNodeModules: false, }), ), ).toEqual({ cmd: "npm", args: ["install"], description: "npm install" }); @@ -192,7 +193,6 @@ describe("detectInstallConfig", () => { input({ packageJson: { dependencies: { react: "^19.0.0" } }, lockfiles: new Set(["pnpm-lock.yaml"]), - hasNodeModules: false, }), ), ).toEqual({ cmd: "pnpm", args: ["install"], description: "pnpm install" }); @@ -202,7 +202,6 @@ describe("detectInstallConfig", () => { input({ packageJson: { dependencies: { react: "^19.0.0" } }, lockfiles: new Set(["bun.lockb"]), - hasNodeModules: false, }), ), ).toEqual({ cmd: "bun", args: ["install"], description: "bun install" }); @@ -212,7 +211,6 @@ describe("detectInstallConfig", () => { input({ packageJson: { dependencies: { react: "^19.0.0" } }, lockfiles: new Set(["yarn.lock"]), - hasNodeModules: false, }), ), ).toEqual({ cmd: "yarn", args: ["install"], description: "yarn install" }); diff --git a/src/utils/build/detect.ts b/src/utils/build/detect.ts index c39a519..e4c9456 100644 --- a/src/utils/build/detect.ts +++ b/src/utils/build/detect.ts @@ -61,8 +61,6 @@ export interface DetectInput { lockfiles: Set; /** Set of additional config-file basenames (e.g. vite.config.ts). */ configFiles: Set; - /** Whether a node_modules/ directory exists at the project root. */ - hasNodeModules: boolean; /** Raw Cargo.toml contents (used to gate the cdm contract flow). Null when absent. */ cargoToml: string | null; } @@ -172,17 +170,19 @@ export function detectContractsType(input: DetectInput): ContractsType | null { /** * Decide whether we need to run an install step before building. Returns the - * install command when the project has dependencies declared but no - * node_modules/ directory, otherwise null. + * install command whenever the project declares any dependencies, otherwise + * null. * - * Rationale: without this check, `dot build` for an uninstalled project falls - * through to `npx vite build` (or similar), which ephemerally downloads the - * framework binary but can't resolve the project's own `vite.config.ts` - * imports — yielding a confusing ERR_MODULE_NOT_FOUND deep in the config - * loader. Auto-installing first eliminates the footgun. + * We install unconditionally (not just when node_modules/ is missing) because + * a stale node_modules/ — e.g. after a branch switch or a teammate bumping the + * lockfile — bypasses the missing-folder guard and lets the build run against + * package versions that don't match the lockfile. The resulting "X is not + * exported by Y" error from Vite/Rollup is opaque and the user has no signal + * that the fix is a re-install. pnpm/yarn/npm are all idempotent when in sync + * (~1s no-op), so the cost is negligible. Skipping install for a deps-free + * package.json is still safe: there's nothing to install. */ export function detectInstallConfig(input: DetectInput): InstallConfig | null { - if (input.hasNodeModules) return null; const pkg = input.packageJson; if (!pkg) return null; const depCount = diff --git a/src/utils/build/runner.ts b/src/utils/build/runner.ts index ce1aed4..8d68447 100644 --- a/src/utils/build/runner.ts +++ b/src/utils/build/runner.ts @@ -76,7 +76,6 @@ export function loadDetectInput(projectDir: string): DetectInput { packageJson, lockfiles, configFiles, - hasNodeModules: existsSync(join(root, "node_modules")), cargoToml, }; } diff --git a/src/utils/deploy/run.test.ts b/src/utils/deploy/run.test.ts index 577988d..31a367e 100644 --- a/src/utils/deploy/run.test.ts +++ b/src/utils/deploy/run.test.ts @@ -62,7 +62,7 @@ const { packageJson: { scripts: { build: "vite build" } }, lockfiles: new Set(), configFiles: new Set(), - hasNodeModules: true, + cargoToml: null, })), detectContractsTypeMock: vi.fn<() => "foundry" | "hardhat" | "cdm" | null>(() => null), runContractsPhaseMock: vi.fn< diff --git a/src/utils/process.test.ts b/src/utils/process.test.ts index cb27d3e..53fedc3 100644 --- a/src/utils/process.test.ts +++ b/src/utils/process.test.ts @@ -87,9 +87,9 @@ describe("runStreamed", () => { ).rejects.toThrow(/\(no output\)/); }); - it("caps the captured tail and reports only the LAST 10 lines on failure (not the first)", async () => { + it("caps the captured tail and reports only the LAST 40 lines on failure (not the first)", async () => { // Generate 60 numbered lines, then exit 1. Tail buffer holds the last - // 50; the error message shows the last 10 of those — so 51..60. + // 40, which is what the error message reports — so 21..60. try { await runStreamed({ cmd: "/bin/sh", @@ -99,15 +99,50 @@ describe("runStreamed", () => { expect.fail("expected rejection"); } catch (err) { const msg = (err as Error).message; - // Last 10 lines must appear. - for (let i = 51; i <= 60; i++) { + // Last 40 lines must appear. + for (let i = 21; i <= 60; i++) { expect(msg).toContain(`line-${i}`); } // Anything earlier must NOT: confirms we're taking the tail, not - // the head, and that the slice is capped at 10. + // the head, and that the buffer is capped at 40. expect(msg).not.toContain("line-1\n"); - expect(msg).not.toContain("line-50\n"); - expect(msg).not.toContain("line-10\n"); + expect(msg).not.toContain("line-20\n"); + } + }); + + it("preserves a Vite/Rollup-style error message that precedes a long stack trace", async () => { + // Realistic shape: vite prints the build banner, a single descriptive + // error line, a code snippet, and finally a ~12-frame stack trace. + // With the older 10-line snippet the actual error was pushed off the + // window by the trailing trace; the wider window keeps it visible. + const script = [ + "echo 'vite v7.3.2 building client environment for production...'", + "echo 'transforming...'", + "echo '✓ 1544 modules transformed.'", + "echo '✗ Build failed in 843ms'", + "echo 'error during build:'", + `echo ' src/utils/contracts.ts (40:9): \"createContractRuntimeFromClient\" is not exported by node_modules/@parity/product-sdk-contracts/dist/index.js'`, + "echo 'file: /tmp/contracts.ts:40:9'", + "echo '38: import type { PolkadotSigner } from \"polkadot-api\";'", + "echo '39: import { keccak256 } from \"@parity/product-sdk-utils\";'", + "echo '40: import { createContractRuntimeFromClient } from \"@parity/product-sdk-contracts\";'", + "echo ' ^'", + "echo '41: import { paseo_asset_hub } from \"@parity/product-sdk-descriptors\";'", + "for i in $(seq 1 12); do echo ' at frame-'$i' (file:///.../rollup/dist/es/shared/node-entry.js:1234:5)'; done", + "exit 1", + ].join("; "); + + try { + await runStreamed({ + cmd: "/bin/sh", + args: ["-c", script], + description: "vite-like-failure", + }); + expect.fail("expected rejection"); + } catch (err) { + const msg = (err as Error).message; + expect(msg).toContain('"createContractRuntimeFromClient" is not exported'); + expect(msg).toContain("at frame-12"); } }); }); diff --git a/src/utils/process.ts b/src/utils/process.ts index a7c0f86..4884dad 100644 --- a/src/utils/process.ts +++ b/src/utils/process.ts @@ -34,8 +34,12 @@ export interface RunStreamedOptions { /** * Spawn a child process, stream every non-empty stdout/stderr line through - * `onData`, and resolve/reject based on exit code. Includes the last ~10 + * `onData`, and resolve/reject based on exit code. Includes the last ~40 * lines of output in the rejection message so failures are diagnosable. + * + * The window is 40 (not 10) because Vite/Rollup and many bundlers print the + * meaningful error first and then a 10–20 line stack trace; a smaller window + * keeps the trace and drops the message that explains what actually broke. */ export async function runStreamed(opts: RunStreamedOptions): Promise { const description = opts.description ?? `${opts.cmd} ${opts.args.join(" ")}`; @@ -49,7 +53,7 @@ export async function runStreamed(opts: RunStreamedOptions): Promise { }); const tail: string[] = []; - const MAX_TAIL = 50; + const MAX_TAIL = 40; const forward = (chunk: Buffer) => { for (const line of chunk.toString().split("\n")) { @@ -71,7 +75,7 @@ export async function runStreamed(opts: RunStreamedOptions): Promise { if (code === 0) { resolvePromise(); } else { - const snippet = tail.slice(-10).join("\n") || "(no output)"; + const snippet = tail.join("\n") || "(no output)"; rejectPromise( new Error( `${failurePrefix} (${description}) with exit code ${code}.\n${snippet}`,