From 92f07f76b50c7b02316b61c1c00e1ab2a8ac63ca Mon Sep 17 00:00:00 2001 From: SylvainDruaux Date: Thu, 22 Oct 2026 23:17:18 +0200 Subject: [PATCH 1/5] refactor(publishing): share one exhaustive sign-in pending check The account service and the sign-in dialog each kept their own list of pending states, so a new state could silently be treated as finished. Both now use accountSignInPending from the shared contract, a switch with a never check, so a new state fails to compile until classified. --- electron/project/publishAccount.ts | 5 +--- shared/ipc/publishAccount.ts | 20 +++++++++++++++ src/features/publishing/PublishAccount.tsx | 3 ++- .../shared/ipc/publishAccountContract.test.ts | 25 ++++++++++++++++++- 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/electron/project/publishAccount.ts b/electron/project/publishAccount.ts index 8a643279..59a608ff 100644 --- a/electron/project/publishAccount.ts +++ b/electron/project/publishAccount.ts @@ -3,6 +3,7 @@ import { assert } from '../../shared/core/assert'; import { ACCOUNT_SIGN_IN_TIMEOUT_MS, ACCOUNT_INSTALL_TIMEOUT_MS, + accountSignInPending, parseAccountRequest, parseCloudflareSignInURL, type AccountRequest, @@ -221,10 +222,6 @@ export class PublishingAccount { } } -function accountSignInPending(state: AccountState): boolean { - return ['installing', 'starting', 'waiting', 'browser'].includes(state.kind); -} - function launchAccountSignIn( root: string, provider: 'github' | 'cloudflare', diff --git a/shared/ipc/publishAccount.ts b/shared/ipc/publishAccount.ts index e626599b..6c2062ff 100644 --- a/shared/ipc/publishAccount.ts +++ b/shared/ipc/publishAccount.ts @@ -64,6 +64,26 @@ export function parseAccountState(input: unknown): AccountState { } } +// Whether a sign-in process is still running: the renderer keeps polling it, and the +// service refuses a second sign-in until it ends. Adding a state must decide this here. +export function accountSignInPending(state: AccountState): boolean { + switch (state.kind) { + case 'installing': + case 'starting': + case 'waiting': + case 'browser': + return true; + case 'complete': + case 'cancelled': + case 'failed': + return false; + default: { + const exhaustive: never = state; + return exhaustive; + } + } +} + export function parseCloudflareSignInURL(input: unknown): string { const value = text(input); assert(value.length <= 4096, 'Cloudflare sign-in URL stays bounded'); diff --git a/src/features/publishing/PublishAccount.tsx b/src/features/publishing/PublishAccount.tsx index b7b369dd..fb3e4518 100644 --- a/src/features/publishing/PublishAccount.tsx +++ b/src/features/publishing/PublishAccount.tsx @@ -5,6 +5,7 @@ import Popup from '../../ui/Popup'; import { ACCOUNT_INSTALL_TIMEOUT_MS, ACCOUNT_SIGN_IN_TIMEOUT_MS, + accountSignInPending, type AccountState, } from '../../../shared/ipc/publishAccount'; @@ -136,7 +137,7 @@ function useAccountSignIn( return; } setState(result); - if (!['installing', 'starting', 'waiting', 'browser'].includes(result.kind)) { + if (!accountSignInPending(result)) { return; } await new Promise((resolve) => { diff --git a/test/shared/ipc/publishAccountContract.test.ts b/test/shared/ipc/publishAccountContract.test.ts index e33bddf3..12b85b8c 100644 --- a/test/shared/ipc/publishAccountContract.test.ts +++ b/test/shared/ipc/publishAccountContract.test.ts @@ -3,7 +3,11 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { parseIpcPayload } from '#dist/shared/ipc/ipcPayloads.js'; -import { parseAccountRequest, parseAccountState } from '#dist/shared/ipc/publishAccount.js'; +import { + accountSignInPending, + parseAccountRequest, + parseAccountState, +} from '#dist/shared/ipc/publishAccount.js'; const id = '00000000-0000-4000-8000-000000000000'; test('GitHub sign-in actions and states round-trip', () => { @@ -88,3 +92,22 @@ test('Cloudflare sign-in accepts only its bounded authorization URL and provider assert.throws(() => parseAccountState({ kind: 'browser', url: invalid })); } }); + +test('only running sign-in states are pending', () => { + const url = 'https://dash.cloudflare.com/oauth2/auth?client_id=test&state=nonce'; + for (const state of [ + { kind: 'installing' }, + { kind: 'starting' }, + { kind: 'waiting', code: 'ABCD-1234' }, + { kind: 'browser', url }, + ]) { + assert.equal(accountSignInPending(parseAccountState(state)), true, state.kind); + } + for (const state of [ + { kind: 'complete' }, + { kind: 'cancelled' }, + { kind: 'failed', message: 'Unavailable' }, + ]) { + assert.equal(accountSignInPending(parseAccountState(state)), false, state.kind); + } +}); From 78b328320e5809efd07b44c55949bcb3a9dff3f3 Mon Sep 17 00:00:00 2001 From: SylvainDruaux Date: Thu, 22 Oct 2026 23:24:07 +0200 Subject: [PATCH 2/5] feat(project): read the package manager from package.json first A project can name its package manager in package.json's packageManager field (e.g. "pnpm@9.12.0"), which is more reliable than guessing from a lockfile. Detection now checks that field first, then the lockfile, then falls back to npm. An unknown or unreadable field falls back the same way. --- electron/handlers/projectHandlers.ts | 3 +- electron/project/packageManager.ts | 45 ++++++++++++++ electron/project/projectFiles.ts | 20 +----- shared/properties/packageManager.ts | 21 +++++++ test/electron/project/packageManager.test.ts | 65 ++++++++++++++++++++ 5 files changed, 134 insertions(+), 20 deletions(-) create mode 100644 electron/project/packageManager.ts create mode 100644 shared/properties/packageManager.ts create mode 100644 test/electron/project/packageManager.test.ts diff --git a/electron/handlers/projectHandlers.ts b/electron/handlers/projectHandlers.ts index 057ea640..89f82e35 100644 --- a/electron/handlers/projectHandlers.ts +++ b/electron/handlers/projectHandlers.ts @@ -11,7 +11,8 @@ import { scaffoldProject } from '../project/scaffold'; import { createStarter } from '../project/starter'; import { isWin, errorMessage } from '../lib/mainHelpers'; import { ensureToolPath, run } from '../lib/nodeTools'; -import { isAstroProject, detectPackageManager } from '../project/projectFiles'; +import { isAstroProject } from '../project/projectFiles'; +import { detectPackageManager } from '../project/packageManager'; import type { MainHost } from './mainHost'; export function registerProjectHandlers( diff --git a/electron/project/packageManager.ts b/electron/project/packageManager.ts new file mode 100644 index 00000000..57fd6efa --- /dev/null +++ b/electron/project/packageManager.ts @@ -0,0 +1,45 @@ +// Which package manager a project uses. Kept apart from projectFiles so Studio's +// publishing services can share it without the project scan. + +import * as fs from 'fs'; +import * as path from 'path'; +import { readJson, parseRecord } from '../lib/mainHelpers'; +import { + parsePackageManagerField, + type PackageManager, +} from '../../shared/properties/packageManager'; + +// The packageManager field in package.json names it outright; otherwise the +// lockfile shows which one last installed; else npm. +export function detectPackageManager(directory: string): PackageManager { + const declared = declaredPackageManager(directory); + if (declared !== undefined) { + return declared; + } + if (fs.existsSync(path.join(directory, 'pnpm-lock.yaml'))) { + return 'pnpm'; + } + if (fs.existsSync(path.join(directory, 'yarn.lock'))) { + return 'yarn'; + } + if ( + fs.existsSync(path.join(directory, 'bun.lockb')) || + fs.existsSync(path.join(directory, 'bun.lock')) + ) { + return 'bun'; + } + return 'npm'; +} + +function declaredPackageManager(directory: string): PackageManager | undefined { + const manifest = path.join(directory, 'package.json'); + if (!fs.existsSync(manifest)) { + return undefined; + } + try { + return parsePackageManagerField(parseRecord(readJson(manifest))['packageManager']); + } catch { + // An unreadable package.json falls back to the lockfile; the install reports it. + return undefined; + } +} diff --git a/electron/project/projectFiles.ts b/electron/project/projectFiles.ts index 5ce3c80f..1af91901 100644 --- a/electron/project/projectFiles.ts +++ b/electron/project/projectFiles.ts @@ -1,6 +1,5 @@ // What a folder holds as an Astro project: its page, layout and component -// files, the route a page serves, and whether its dependencies are installed -// and with which package manager. +// files, the route a page serves, and whether its dependencies are installed. import { MAIN_LIMITS, directoryBudget } from '../lib/mainLimits'; import { toRecord } from '../../shared/core/record'; @@ -75,20 +74,3 @@ export function hasDependencies(projectPath: string) { const binName = isWin ? 'astro.cmd' : 'astro'; return fs.existsSync(path.join(projectPath, 'node_modules', '.bin', binName)); } - -// Detects the project's package manager from its lockfile. -export function detectPackageManager(directory: string) { - if (fs.existsSync(path.join(directory, 'pnpm-lock.yaml'))) { - return 'pnpm'; - } - if (fs.existsSync(path.join(directory, 'yarn.lock'))) { - return 'yarn'; - } - if ( - fs.existsSync(path.join(directory, 'bun.lockb')) || - fs.existsSync(path.join(directory, 'bun.lock')) - ) { - return 'bun'; - } - return 'npm'; -} diff --git a/shared/properties/packageManager.ts b/shared/properties/packageManager.ts new file mode 100644 index 00000000..0b271fcd --- /dev/null +++ b/shared/properties/packageManager.ts @@ -0,0 +1,21 @@ +// The package managers Stacki runs for a project, and how package.json names one +// in its packageManager field: "pnpm@9.12.0", optionally followed by "+sha512.…". + +export const PACKAGE_MANAGERS = ['npm', 'pnpm', 'yarn', 'bun'] as const; +export type PackageManager = (typeof PACKAGE_MANAGERS)[number]; + +const FIELD_CHARS_MAX = 256; + +// The manager the field names, or undefined when it is absent, malformed or names +// one Stacki does not run, so detection falls back to the project's lockfile. +export function parsePackageManagerField(input: unknown): PackageManager | undefined { + if (typeof input !== 'string') { + return undefined; + } + if (input.length > FIELD_CHARS_MAX) { + return undefined; + } + const at = input.indexOf('@'); + const name = at === -1 ? input : input.slice(0, at); + return PACKAGE_MANAGERS.find((manager) => manager === name); +} diff --git a/test/electron/project/packageManager.test.ts b/test/electron/project/packageManager.test.ts new file mode 100644 index 00000000..07398d43 --- /dev/null +++ b/test/electron/project/packageManager.test.ts @@ -0,0 +1,65 @@ +// Goal: a project's package manager comes from package.json's packageManager field when it +// names one Stacki runs, then from its lockfile, then npm. +// Method: parse valid, versioned, unknown, malformed and oversized fields, then detect inside +// temporary project folders that combine fields and lockfiles. Nothing is installed. +import assert from 'node:assert/strict'; +import test, { type TestContext } from 'node:test'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { detectPackageManager } from '#dist/electron/project/packageManager.js'; +import { parsePackageManagerField } from '#dist/shared/properties/packageManager.js'; + +async function project(context: TestContext, files: Readonly>) { + const directory = await mkdtemp(path.join(tmpdir(), 'stacki-package-manager-')); + context.after(() => rm(directory, { recursive: true, force: true })); + for (const [name, content] of Object.entries(files)) { + await writeFile(path.join(directory, name), content); + } + return directory; +} + +test('the packageManager field names a manager with or without a version', () => { + assert.equal(parsePackageManagerField('pnpm@9.12.0'), 'pnpm'); + assert.equal(parsePackageManagerField('yarn@4.5.0+sha512.abc123'), 'yarn'); + assert.equal(parsePackageManagerField('bun@1.1.30'), 'bun'); + assert.equal(parsePackageManagerField('npm'), 'npm'); +}); + +test('the packageManager field names nothing when unknown, malformed or oversized', () => { + for (const input of [ + undefined, + 7, + '', + '@9.0.0', + 'deno@2.0.0', + 'PNPM@9.0.0', + ` pnpm@9.0.0`, + `pnpm@${'9'.repeat(256)}`, + ]) { + assert.equal(parsePackageManagerField(input), undefined, String(input)); + } +}); + +test('the packageManager field outranks the lockfile', async (context) => { + const directory = await project(context, { + 'package.json': JSON.stringify({ packageManager: 'pnpm@9.12.0' }), + 'package-lock.json': '{}', + 'yarn.lock': '', + }); + assert.equal(detectPackageManager(directory), 'pnpm'); +}); + +test('lockfiles decide when the field is absent, unknown or unreadable', async (context) => { + const cases = [ + [{ 'package.json': '{}', 'pnpm-lock.yaml': '' }, 'pnpm'], + [{ 'package.json': JSON.stringify({ packageManager: 'deno@2.0.0' }), 'yarn.lock': '' }, 'yarn'], + [{ 'package.json': '{ not json', 'bun.lock': '' }, 'bun'], + [{ 'bun.lockb': '' }, 'bun'], + [{ 'package.json': '{}' }, 'npm'], + [{}, 'npm'], + ] as const; + for (const [files, expected] of cases) { + assert.equal(detectPackageManager(await project(context, files)), expected); + } +}); From 58154b24d2a219649d3c73f259f5234f8ffde019 Mon Sep 17 00:00:00 2001 From: SylvainDruaux Date: Fri, 23 Oct 2026 11:14:20 +0200 Subject: [PATCH 3/5] feat(publishing): install Wrangler with the site's package manager Cloudflare sign-in installed a missing Wrangler always with npm, so sites using pnpm, Yarn or Bun ended up with a second lockfile, and cancelling could leave node_modules half installed. It now installs with the site's package manager, and cancelling skips sign-in but lets the install finish. The dialog stays on one screen from installation to browser sign-in or Try again, and says when the site's dependencies changed. A Wrangler removed while Stacki runs is detected as missing again instead of failing to start. --- docs/contracts.md | 15 +- docs/publishing.md | 10 +- electron/project/publishAccount.ts | 190 +++++++++++++----- electron/project/publishExecutable.ts | 17 +- shared/properties/packageManager.ts | 18 ++ src/features/publishing/PublishAccount.tsx | 102 ++++++++-- studio/scripts/shared-source.json | 2 + test/electron/project/packageManager.test.ts | 13 +- .../project/publishAccountInstall.test.ts | 170 ++++++++++++---- .../publishing/publishAccountInstall.test.js | 69 +++++-- 10 files changed, 467 insertions(+), 139 deletions(-) diff --git a/docs/contracts.md b/docs/contracts.md index 51ac3772..d786eca4 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -702,12 +702,15 @@ smaller patches retain binary/rename metadata and validated hunk line numbers fo ## Project publishing -`publish:account` reports `installing` while Cloudflare sign-in installs a missing project Wrangler -with `npm install --save-dev wrangler`. Only an explicit sign-in start can trigger installation; -discovery and status polling cannot. The same session continues to browser sign-in after a successful -install. Cancellation, a ten-minute installation timeout, or excessive output stops the process tree -and prevents login. Installer output is discarded, and failures expose only sanitized messages. -Login retains its separate five-minute timeout. Existing project/workspace dependencies skip setup. +`publish:account` reports `installing` while Cloudflare sign-in adds a missing project Wrangler as +a development dependency with the site's package manager (package.json's `packageManager` field, +else its lockfile, else npm). Only an explicit sign-in start can trigger installation; discovery +and status polling cannot. The same session continues to browser sign-in after a successful +install. Cancelling skips login but lets the package manager finish, so node_modules is never left +half written; another sign-in is refused until it does. A ten-minute installation timeout or +excessive output stops the process tree and prevents login. Installer output is discarded, and +failures expose only sanitized messages. Login retains its separate five-minute timeout. Existing +project/workspace dependencies skip setup. `.stacki/component-sources.json` registers bounded, read-only installed component roots for both hosts. `shared/properties/componentSources.ts` parses package names, relative directories and diff --git a/docs/publishing.md b/docs/publishing.md index 1b282c8d..9199a975 100644 --- a/docs/publishing.md +++ b/docs/publishing.md @@ -60,11 +60,11 @@ to Studio or committed to the project. Cloudflare Pages needs an existing Pages project, account ID, production branch, and build output directory (usually `dist`). Choosing **Add account** for Cloudflare in Desktop or local -Studio installs a missing project Wrangler with `npm install --save-dev wrangler`, then starts -browser sign-in automatically. The dialog shows installation progress and Cancel stops the -installer. This updates the site's package manifest and lockfile so future deployments can use -the same dependency. Existing project or workspace installations are reused. Installation failures -can be retried; account discovery and deployment never install packages implicitly. +Studio installs a missing project Wrangler with the site's package manager, then starts browser +sign-in automatically. This updates the site's package manifest and lockfile so future +deployments can use the same dependency. Cancel during installation skips sign-in but lets the +installation finish. Existing project or workspace installations are reused. Installation +failures can be retried; account discovery and deployment never install packages implicitly. Alternatively, install Wrangler and run `wrangler login` once locally, or provide `CLOUDFLARE_API_TOKEN` in the server environment. The account ID is explicitly supplied on every deployment, so another project's account choice does not change this project's target. diff --git a/electron/project/publishAccount.ts b/electron/project/publishAccount.ts index 59a608ff..27f2eb97 100644 --- a/electron/project/publishAccount.ts +++ b/electron/project/publishAccount.ts @@ -9,16 +9,23 @@ import { type AccountRequest, type AccountState, } from '../../shared/ipc/publishAccount'; +import { packageManagerLabel, type PackageManager } from '../../shared/properties/packageManager'; import { ensureToolPath } from '../lib/nodeTools'; +import { detectPackageManager } from './packageManager'; import { projectHasWrangler, resolvePublishExecutable } from './publishExecutable'; import { publishEnvironment, stopPublishProcess } from './publishCommands'; +// What a launched process does: install Wrangler with the site's manager, or sign in. +export type LaunchStep = + | { readonly kind: 'installing'; readonly packageManager: PackageManager } + | { readonly kind: 'starting' }; + interface Session { readonly id: string; readonly root: string; readonly provider: 'github' | 'cloudflare'; readonly process: ChildProcessWithoutNullStreams; - readonly phase: 'installing' | 'starting'; + readonly step: LaunchStep; state: AccountState; output: string; outputChars: number; @@ -29,7 +36,7 @@ const INSTALL_OUTPUT_CHARS_MAX = 1_048_576; type Launch = ( root: string, provider: 'github' | 'cloudflare', - phase: Session['phase'], + step: LaunchStep, ) => ChildProcessWithoutNullStreams; // One login can own the CLI credential store at a time. Raw output never leaves this service. @@ -39,12 +46,13 @@ export class PublishingAccount { constructor( private readonly launch: Launch = launchAccountSignIn, private readonly hasWrangler: (root: string) => boolean = projectHasWrangler, + private readonly packageManager: (root: string) => PackageManager = detectPackageManager, ) {} request(root: string, input: AccountRequest): AccountState { const request = parseAccountRequest(input); - const session = this.session; const provider = request.provider ?? 'github'; + const session = this.session; if (session?.id === request.id && session.root === root && session.provider === provider) { if (request.action === 'cancel' && accountSignInPending(session.state)) { this.finish(session, { kind: 'cancelled' }); @@ -54,24 +62,52 @@ export class PublishingAccount { if (request.action !== 'start') { return { kind: 'failed', message: 'This sign-in session has ended. Please try again.' }; } - if (session && accountSignInPending(session.state)) { + const busy = this.busy(); + if (busy !== undefined) { + return busy; + } + const step: LaunchStep = + provider === 'cloudflare' && !this.hasWrangler(root) + ? { kind: 'installing', packageManager: this.packageManager(root) } + : { kind: 'starting' }; + return this.start(root, request.id, provider, step); + } + + // A pending sign-in owns the CLI credentials, and a cancelled installation still + // writing the site must finish before another one can start. + private busy(): AccountState | undefined { + const session = this.session; + if (session === undefined) { + return undefined; + } + if (accountSignInPending(session.state)) { return { kind: 'failed', message: 'Finish or cancel the account sign-in already in progress.', }; } - const phase = provider === 'cloudflare' && !this.hasWrangler(root) ? 'installing' : 'starting'; - return this.start(root, request.id, provider, phase); + if (session.step.kind === 'installing' && running(session.process)) { + return { + kind: 'failed', + message: 'Wrangler is still installing in the background. Try again in a moment.', + }; + } + return undefined; } private start( root: string, id: string, provider: 'github' | 'cloudflare', - phase: Session['phase'], + step: LaunchStep, ): AccountState { - const child = this.launch(root, provider, phase); - const label = provider === 'github' ? 'GitHub' : 'Cloudflare'; + let child: ChildProcessWithoutNullStreams; + try { + child = this.launch(root, provider, step); + } catch { + // On Windows a missing tool is found while resolving it, before any process starts. + return { kind: 'failed', message: launchFailure(provider, step) }; + } const stop = () => { stopAccountSignIn(child, provider); }; @@ -82,34 +118,43 @@ export class PublishingAccount { root, provider, process: child, - phase, - state: { kind: phase }, + step, + state: { kind: step.kind }, output: '', outputChars: 0, timer: undefined, }; this.session = session; session.timer = setTimeout( - () => { - this.finish(session, { - kind: 'failed', - message: - phase === 'installing' - ? 'Wrangler installation timed out. Check your connection and try again.' - : `${label} sign-in expired. Please try again.`, - }); - }, - phase === 'installing' ? ACCOUNT_INSTALL_TIMEOUT_MS : ACCOUNT_SIGN_IN_TIMEOUT_MS, + () => this.expire(session), + step.kind === 'installing' ? ACCOUNT_INSTALL_TIMEOUT_MS : ACCOUNT_SIGN_IN_TIMEOUT_MS, ); session.timer.unref(); this.watch(session); return session.state; } + // A pending session fails at its timeout. A cancelled installation left to finish in + // the background is stopped at the same timeout, so it stays bounded too. + private expire(session: Session): void { + if (accountSignInPending(session.state)) { + const label = session.provider === 'github' ? 'GitHub' : 'Cloudflare'; + this.finish(session, { + kind: 'failed', + message: + session.step.kind === 'installing' + ? 'Wrangler installation timed out. Check your connection and try again.' + : `${label} sign-in expired. Please try again.`, + }); + return; + } + if (running(session.process)) { + stopAccountSignIn(session.process, session.provider); + } + } + private watch(session: Session): void { const child = session.process; - const label = session.provider === 'github' ? 'GitHub' : 'Cloudflare'; - const tool = session.provider === 'github' ? 'gh' : 'Wrangler'; child.stdout.setEncoding('utf8'); child.stderr.setEncoding('utf8'); child.stdout.on('data', (chunk: unknown) => this.read(session, chunk)); @@ -117,10 +162,7 @@ export class PublishingAccount { child.on('error', () => this.finish(session, { kind: 'failed', - message: - session.phase === 'installing' - ? 'Could not start Wrangler installation. Install Node.js and npm, then try again.' - : `Could not start ${label} sign-in. Install ${tool} and try again.`, + message: launchFailure(session.provider, session.step), }), ); child.on('close', (code) => this.closed(session, code ?? undefined)); @@ -130,14 +172,23 @@ export class PublishingAccount { } private closed(session: Session, code: number | undefined): void { - if (this.session !== session || !accountSignInPending(session.state)) { + if (this.session !== session) { + return; + } + if (!accountSignInPending(session.state)) { + // A cancelled installation that finished in the background needs no timeout now. + clearTimeout(session.timer); return; } - if (session.phase === 'installing') { + if (session.step.kind === 'installing') { if (code === 0) { clearTimeout(session.timer); // Session identity prevents late installer events from completing the subsequent login. - this.start(session.root, session.id, session.provider, 'starting'); + const login = this.start(session.root, session.id, session.provider, { kind: 'starting' }); + if (this.session === session) { + // Login could not launch, so no new session replaced this one. + this.finish(session, login); + } } else { this.finish(session, { kind: 'failed', @@ -169,11 +220,11 @@ export class PublishingAccount { } assert(typeof chunk === 'string', 'Account sign-in streams use UTF-8 text'); assert(session.output.length <= OUTPUT_CHARS_MAX, 'Account sign-in output stays bounded'); - const label = - session.phase === 'installing' - ? 'Wrangler installation' - : `${session.provider === 'github' ? 'GitHub' : 'Cloudflare'} sign-in`; - const limit = session.phase === 'installing' ? INSTALL_OUTPUT_CHARS_MAX : OUTPUT_CHARS_MAX; + const installing = session.step.kind === 'installing'; + const label = installing + ? 'Wrangler installation' + : `${session.provider === 'github' ? 'GitHub' : 'Cloudflare'} sign-in`; + const limit = installing ? INSTALL_OUTPUT_CHARS_MAX : OUTPUT_CHARS_MAX; if (chunk.length > limit - session.outputChars) { this.finish(session, { kind: 'failed', @@ -183,7 +234,7 @@ export class PublishingAccount { } session.outputChars += chunk.length; // Installation logs can contain registry credentials; count them without retaining them. - if (session.phase === 'installing') { + if (installing) { return; } session.output += chunk; @@ -214,21 +265,61 @@ export class PublishingAccount { assert(this.session === session, 'Only the current sign-in session can finish'); session.state = state; session.output = ''; + if (session.step.kind === 'installing') { + if (state.kind === 'cancelled') { + // Stopping a package manager mid-install can leave node_modules half written, so + // a cancelled installation finishes in the background, bounded by its timeout. + return; + } + } clearTimeout(session.timer); session.timer = undefined; - if (session.process.exitCode === null && session.process.signalCode === null) { + if (running(session.process)) { stopAccountSignIn(session.process, session.provider); } } } +function running(child: ChildProcessWithoutNullStreams): boolean { + return child.exitCode === null && child.signalCode === null; +} + +function launchFailure(provider: 'github' | 'cloudflare', step: LaunchStep): string { + if (step.kind === 'installing') { + const label = packageManagerLabel(step.packageManager); + return `Could not run ${label} to install Wrangler. Install ${label}, then try again.`; + } + return provider === 'github' + ? 'Could not start GitHub sign-in. Install gh and try again.' + : 'Could not start Cloudflare sign-in. Install Wrangler and try again.'; +} + +// Adds the latest Wrangler as a development dependency with the site's manager. +export function wranglerInstallArguments(manager: PackageManager): readonly string[] { + switch (manager) { + case 'npm': + return ['install', '--save-dev', 'wrangler']; + case 'pnpm': + // By default `pnpm` blocks dependency build scripts and, since v11, exits with an error + // when it does, though Wrangler is installed. Keep them blocked; just don't fail. + return ['add', '--save-dev', '--config.strict-dep-builds=false', 'wrangler']; + case 'yarn': + case 'bun': + return ['add', '--dev', 'wrangler']; + default: { + const exhaustive: never = manager; + return exhaustive; + } + } +} + function launchAccountSignIn( root: string, provider: 'github' | 'cloudflare', - phase: Session['phase'], + step: LaunchStep, ): ChildProcessWithoutNullStreams { if (provider === 'cloudflare') { - return launchCloudflareSignIn(root, phase); + return launchCloudflareSignIn(root, step); } ensureToolPath(); const environment: NodeJS.ProcessEnv = { ...process.env, NO_COLOR: '1', GH_PROMPT_DISABLED: '1' }; @@ -257,17 +348,18 @@ function launchAccountSignIn( ); } -function launchCloudflareSignIn( - root: string, - phase: Session['phase'], -): ChildProcessWithoutNullStreams { +function launchCloudflareSignIn(root: string, step: LaunchStep): ChildProcessWithoutNullStreams { ensureToolPath(); - const command = resolvePublishExecutable(root, { - executable: phase === 'installing' ? 'npm' : 'wrangler', - arguments: - phase === 'installing' ? ['install', '--save-dev', 'wrangler'] : ['login', '--browser=false'], - environment: {}, - }); + const command = resolvePublishExecutable( + root, + step.kind === 'installing' + ? { + executable: step.packageManager, + arguments: wranglerInstallArguments(step.packageManager), + environment: {}, + } + : { executable: 'wrangler', arguments: ['login', '--browser=false'], environment: {} }, + ); return spawn(command.executable, [...command.arguments], { cwd: root, env: { ...publishEnvironment(), NO_COLOR: '1', WRANGLER_SEND_METRICS: 'false' }, diff --git a/electron/project/publishExecutable.ts b/electron/project/publishExecutable.ts index 7044268e..65f0475d 100644 --- a/electron/project/publishExecutable.ts +++ b/electron/project/publishExecutable.ts @@ -3,13 +3,15 @@ import { existsSync, readFileSync, realpathSync } from 'node:fs'; import path from 'node:path'; import { record, text } from '../../shared/core/boundary'; import type { PublishCommand } from '../../shared/properties/publishCommands'; +import type { PackageManager } from '../../shared/properties/packageManager'; export function projectHasWrangler(root: string): boolean { const resolver = createRequire(path.join(root, 'package.json')); try { // Resolution from the site also supports dependencies hoisted by a workspace package manager. - resolver.resolve('wrangler/package.json'); - return true; + // Node caches resolved paths for the life of the process, so a Wrangler removed since the + // last lookup still resolves; only a manifest that exists now counts. + return existsSync(resolver.resolve('wrangler/package.json')); } catch (error: unknown) { if (error instanceof Error && 'code' in error && error.code === 'MODULE_NOT_FOUND') { return false; @@ -18,9 +20,14 @@ export function projectHasWrangler(root: string): boolean { } } +// A publish command, or a package manager installing a publishing tool. +export interface ExecutableCommand extends Omit { + readonly executable: PublishCommand['executable'] | PackageManager; +} + export function resolvePublishExecutable( root: string, - command: PublishCommand, + command: ExecutableCommand, ): { readonly executable: string; readonly arguments: readonly string[]; @@ -97,6 +104,10 @@ function installedPublishTool(root: string, executable: string): string | undefi } catch { continue; } + // A cached resolution can outlive a removed package; look further instead. + if (!existsSync(manifest)) { + continue; + } return publishEntry(path.dirname(manifest)); } return undefined; diff --git a/shared/properties/packageManager.ts b/shared/properties/packageManager.ts index 0b271fcd..e22aa6f6 100644 --- a/shared/properties/packageManager.ts +++ b/shared/properties/packageManager.ts @@ -6,6 +6,24 @@ export type PackageManager = (typeof PACKAGE_MANAGERS)[number]; const FIELD_CHARS_MAX = 256; +// The name people know each manager by, for messages. +export function packageManagerLabel(manager: PackageManager): string { + switch (manager) { + case 'npm': + return 'npm'; + case 'pnpm': + return 'pnpm'; + case 'yarn': + return 'Yarn'; + case 'bun': + return 'Bun'; + default: { + const exhaustive: never = manager; + return exhaustive; + } + } +} + // The manager the field names, or undefined when it is absent, malformed or names // one Stacki does not run, so detection falls back to the project's lockfile. export function parsePackageManagerField(input: unknown): PackageManager | undefined { diff --git a/src/features/publishing/PublishAccount.tsx b/src/features/publishing/PublishAccount.tsx index fb3e4518..ee43473f 100644 --- a/src/features/publishing/PublishAccount.tsx +++ b/src/features/publishing/PublishAccount.tsx @@ -24,12 +24,18 @@ export function AddPublishAccount(props: Props) { const label = props.provider === 'github' ? 'GitHub' : 'Cloudflare'; const [attempt, setAttempt] = useState(0); const state = useAccountSignIn(props.projectPath, props.provider, attempt); + // The attempt that installed Wrangler, so its later steps can say the site changed. + const [installedAttempt, setInstalledAttempt] = useState(undefined); const { connected } = props; useEffect(() => { if (state.kind === 'complete') { connected(); } - }, [state.kind, connected]); + if (state.kind === 'installing') { + setInstalledAttempt(attempt); + } + }, [state.kind, connected, attempt]); + const retry = () => setAttempt(attempt + 1); return (
Add {label} account
- {state.kind === 'installing' ? ( -

- Installing Wrangler in this project. Cloudflare sign-in will start automatically… -

+ {props.provider === 'cloudflare' ? ( + ) : state.kind === 'waiting' ? ( - ) : state.kind === 'browser' ? ( - ) : state.kind === 'failed' ? ( <> - @@ -161,8 +167,15 @@ function useAccountSignIn( return state; } -function AccountBrowserSignIn({ url }: { readonly url: string }) { +// One screen for the whole Cloudflare sign-in: installing, starting, waiting and failing only +// change the button and the status line, so nothing flashes past between steps. +function CloudflareSignIn(props: { + readonly state: AccountState; + readonly installed: boolean; + readonly retry: () => void; +}) { const [error, setError] = useState(''); + const { state } = props; return ( <>

Sign in to Cloudflare and approve access to connect your accounts.

@@ -170,20 +183,65 @@ function AccountBrowserSignIn({ url }: { readonly url: string }) { To use a different login, switch accounts in Cloudflare before approving access. This updates the Cloudflare login used by local publishing.

- -

Waiting for sign-in…

+ {props.installed && state.kind !== 'installing' && ( +

Wrangler was added to this site’s development dependencies.

+ )} + {state.kind === 'failed' ? ( + + ) : ( + + )} + {error && } ); } + +function CloudflareSignInStatus({ state }: { readonly state: AccountState }) { + switch (state.kind) { + case 'installing': + return ( +

+ Installing Wrangler in this site… Cancelling skips sign-in but lets the installation + finish. +

+ ); + case 'starting': + return

Starting Cloudflare sign-in…

; + case 'waiting': + case 'browser': + return

Waiting for sign-in…

; + case 'complete': + return

Account connected.

; + case 'cancelled': + return

Sign-in cancelled.

; + case 'failed': + // The cause goes to the app's error toast; the screen only says sign-in stopped. + return ( + <> +

Sign-in did not finish.

+ + + ); + default: { + const exhaustive: never = state; + return exhaustive; + } + } +} diff --git a/studio/scripts/shared-source.json b/studio/scripts/shared-source.json index deb14632..472ba642 100644 --- a/studio/scripts/shared-source.json +++ b/studio/scripts/shared-source.json @@ -73,6 +73,7 @@ "electron/lib/componentSources.ts", "electron/properties/componentImports.ts", "electron/lib/installedPackages.ts", + "electron/project/packageManager.ts", "electron/project/publishCommands.ts", "electron/project/publishRecords.ts", "electron/project/publishWorkerConfig.ts", @@ -164,6 +165,7 @@ "shared/properties/propSchema.ts", "shared/properties/propertyEditing.ts", "shared/properties/propertyOptions.ts", + "shared/properties/packageManager.ts", "shared/properties/publishCommands.ts", "shared/properties/publishConfig.ts", "shared/ipc/publishAccount.ts", diff --git a/test/electron/project/packageManager.test.ts b/test/electron/project/packageManager.test.ts index 07398d43..9d2fd6df 100644 --- a/test/electron/project/packageManager.test.ts +++ b/test/electron/project/packageManager.test.ts @@ -8,7 +8,11 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { detectPackageManager } from '#dist/electron/project/packageManager.js'; -import { parsePackageManagerField } from '#dist/shared/properties/packageManager.js'; +import { + PACKAGE_MANAGERS, + packageManagerLabel, + parsePackageManagerField, +} from '#dist/shared/properties/packageManager.js'; async function project(context: TestContext, files: Readonly>) { const directory = await mkdtemp(path.join(tmpdir(), 'stacki-package-manager-')); @@ -41,6 +45,13 @@ test('the packageManager field names nothing when unknown, malformed or oversize } }); +test('every package manager has the name people know it by', () => { + assert.deepEqual( + PACKAGE_MANAGERS.map((manager) => packageManagerLabel(manager)), + ['npm', 'pnpm', 'Yarn', 'Bun'], + ); +}); + test('the packageManager field outranks the lockfile', async (context) => { const directory = await project(context, { 'package.json': JSON.stringify({ packageManager: 'pnpm@9.12.0' }), diff --git a/test/electron/project/publishAccountInstall.test.ts b/test/electron/project/publishAccountInstall.test.ts index bc52c910..51a84bfa 100644 --- a/test/electron/project/publishAccountInstall.test.ts +++ b/test/electron/project/publishAccountInstall.test.ts @@ -1,6 +1,9 @@ -// Goal: missing project Wrangler installs once before login and remains cancellable and bounded. +// Goal: missing project Wrangler installs once with the site's package manager before login, which +// then starts in the same session. Installation stays bounded, and cancelling it skips login +// without interrupting the package manager. // Method: run deterministic child processes through the real session manager and intercept only -// process launch when checking npm arguments. No packages are downloaded and no account is changed. +// process launch when checking install arguments. No packages are downloaded and no account is +// changed. Timeouts use mocked timers. import assert from 'node:assert/strict'; import test, { type TestContext } from 'node:test'; import childProcess, { @@ -12,25 +15,30 @@ import { once } from 'node:events'; import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; import path from 'node:path'; -import { PublishingAccount } from '#dist/electron/project/publishAccount.js'; +import { + PublishingAccount, + wranglerInstallArguments, +} from '#dist/electron/project/publishAccount.js'; import { projectHasWrangler } from '#dist/electron/project/publishExecutable.js'; import { setToolPathHome } from '#dist/electron/lib/nodeTools.js'; import { ACCOUNT_INSTALL_TIMEOUT_MS } from '#dist/shared/ipc/publishAccount.js'; const id = '00000000-0000-4000-8000-000000000000'; +const other = '11111111-1111-4111-8111-111111111111'; const request = { id, provider: 'cloudflare' as const }; const root = process.cwd(); const browserURL = 'https://dash.cloudflare.com/oauth2/auth?client_id=test&state=nonce'; const login = `process.stdout.write('${browserURL}\\n'); setInterval(() => {}, 1000);`; +const endless = "process.stdout.write('ready'); setInterval(() => {}, 1000);"; function fixture(context: TestContext, script: string) { const children: ChildProcessWithoutNullStreams[] = []; - const phases: string[] = []; + const steps: string[] = []; const account = new PublishingAccount( - (_root, provider, phase) => { + (_root, provider, step) => { assert.equal(provider, 'cloudflare'); - phases.push(phase); - const child = spawn(process.execPath, ['-e', phase === 'installing' ? script : login], { + steps.push(step.kind); + const child = spawn(process.execPath, ['-e', step.kind === 'installing' ? script : login], { stdio: 'pipe', shell: false, detached: process.platform !== 'win32', @@ -39,30 +47,46 @@ function fixture(context: TestContext, script: string) { return child; }, () => false, + () => 'pnpm', ); - context.after(() => account.request(root, { ...request, action: 'cancel' })); - assert.deepEqual(account.request(root, { ...request, action: 'start' }), { kind: 'installing' }); + context.after(() => { + account.request(root, { ...request, action: 'cancel' }); + for (const child of children) { + child.kill('SIGKILL'); + } + }); + assert.deepEqual(account.request(root, { ...request, action: 'start' }), { + kind: 'installing', + }); const installer = children[0]; assert.ok(installer); - return { account, children, installer, phases }; + return { account, children, installer, steps }; } +test('the install uses the package manager detected for the site', () => { + const managers: string[] = []; + const account = new PublishingAccount( + (_root, _provider, step) => { + assert.equal(step.kind, 'installing'); + managers.push(step.packageManager); + throw new Error('the test only inspects the launch step'); + }, + () => false, + () => 'yarn', + ); + account.request(root, { ...request, action: 'start' }); + assert.deepEqual(managers, ['yarn']); +}); + test('successful installation automatically begins login in the same session', async (context) => { - const { account, children, installer, phases } = fixture(context, "console.log('installed');"); + const { account, children, installer, steps } = fixture(context, "console.log('installed');"); assert.equal(account.request(root, { ...request, action: 'start' }).kind, 'installing'); - assert.equal( - account.request(root, { - ...request, - id: '11111111-1111-4111-8111-111111111111', - action: 'start', - }).kind, - 'failed', - ); + assert.equal(account.request(root, { ...request, id: other, action: 'start' }).kind, 'failed'); await once(installer, 'close'); const child = children[1]; assert.ok(child); await once(child.stdout, 'data'); - assert.deepEqual(phases, ['installing', 'starting']); + assert.deepEqual(steps, ['installing', 'starting']); assert.deepEqual(account.request(root, { ...request, action: 'status' }), { kind: 'browser', url: browserURL, @@ -74,60 +98,98 @@ test('successful installation automatically begins login in the same session', a await once(child, 'close'); }); -test('failed installation never starts login or exposes npm output', async (context) => { - const { account, installer, phases } = fixture( +test('failed installation never starts login or exposes installer output', async (context) => { + const { account, installer, steps } = fixture( context, "process.stderr.write('registry-secret'); process.exitCode = 1;", ); await once(installer, 'close'); - assert.deepEqual(phases, ['installing']); + assert.deepEqual(steps, ['installing']); const state = account.request(root, { ...request, action: 'status' }); assert.equal(state.kind, 'failed'); assert.equal(JSON.stringify(state).includes('registry-secret'), false); - const retry = { ...request, id: '11111111-1111-4111-8111-111111111111' }; + const retry = { ...request, id: other }; assert.equal(account.request(root, { ...retry, action: 'start' }).kind, 'installing'); account.request(root, { ...retry, action: 'cancel' }); }); -test('missing npm reports setup failure without retrying or leaking paths', async (context) => { - const directory = await mkdtemp(path.join(tmpdir(), 'stacki-missing-npm-')); +test('a missing package manager reports which one to install', async (context) => { + const directory = await mkdtemp(path.join(tmpdir(), 'stacki-missing-manager-')); context.after(() => rm(directory, { recursive: true, force: true })); - const child = spawn(path.join(directory, 'missing-npm'), [], { stdio: 'pipe', shell: false }); + const child = spawn(path.join(directory, 'missing-pnpm'), [], { stdio: 'pipe', shell: false }); const account = new PublishingAccount( () => child, () => false, + () => 'pnpm', ); const closed = new Promise((resolve) => child.once('close', () => resolve())); assert.equal(account.request(directory, { ...request, action: 'start' }).kind, 'installing'); await closed; assert.deepEqual(account.request(directory, { ...request, action: 'status' }), { kind: 'failed', - message: 'Could not start Wrangler installation. Install Node.js and npm, then try again.', + message: 'Could not run pnpm to install Wrangler. Install pnpm, then try again.', + }); +}); + +test('a package manager that cannot be resolved fails without a session', () => { + const account = new PublishingAccount( + () => { + throw new Error('Install bun in this project.'); + }, + () => false, + () => 'bun', + ); + assert.deepEqual(account.request(root, { ...request, action: 'start' }), { + kind: 'failed', + message: 'Could not run Bun to install Wrangler. Install Bun, then try again.', + }); + // No session was left pending, so a retry reports the same cause, not a sign-in in progress. + assert.deepEqual(account.request(root, { ...request, id: other, action: 'start' }), { + kind: 'failed', + message: 'Could not run Bun to install Wrangler. Install Bun, then try again.', }); }); -test('cancelling installation stops its child and never starts login', async (context) => { - const { account, installer, phases } = fixture( +test('cancelling installation skips login but lets the installer finish', async (context) => { + const { account, installer, steps } = fixture( context, - "process.stdout.write('ready'); setInterval(() => {}, 1000);", + "process.stdout.write('ready'); setTimeout(() => {}, 300);", ); await once(installer.stdout, 'data'); assert.equal(account.request(root, { ...request, action: 'cancel' }).kind, 'cancelled'); + assert.ok(installer.exitCode === null, 'the installer keeps running after cancel'); + assert.ok(installer.signalCode === null, 'the installer is not signalled on cancel'); + assert.deepEqual(account.request(root, { ...request, id: other, action: 'start' }), { + kind: 'failed', + message: 'Wrangler is still installing in the background. Try again in a moment.', + }); await once(installer, 'close'); - assert.deepEqual(phases, ['installing']); + assert.equal(installer.exitCode, 0); + assert.deepEqual(steps, ['installing']); + assert.equal(account.request(root, { ...request, action: 'status' }).kind, 'cancelled'); + const retry = { ...request, id: other }; + assert.equal(account.request(root, { ...retry, action: 'start' }).kind, 'installing'); + account.request(root, { ...retry, action: 'cancel' }); +}); + +test('a cancelled installation is still stopped at its timeout', async (context) => { + context.mock.timers.enable({ apis: ['setTimeout'] }); + const { account, installer, steps } = fixture(context, endless); + await once(installer.stdout, 'data'); + assert.equal(account.request(root, { ...request, action: 'cancel' }).kind, 'cancelled'); + context.mock.timers.tick(ACCOUNT_INSTALL_TIMEOUT_MS); + await once(installer, 'close'); + assert.deepEqual(steps, ['installing']); assert.equal(account.request(root, { ...request, action: 'status' }).kind, 'cancelled'); }); test('installation timeout stops the process without starting login', async (context) => { context.mock.timers.enable({ apis: ['setTimeout'] }); - const { account, installer, phases } = fixture( - context, - "process.stdout.write('ready'); setInterval(() => {}, 1000);", - ); + const { account, installer, steps } = fixture(context, endless); await once(installer.stdout, 'data'); context.mock.timers.tick(ACCOUNT_INSTALL_TIMEOUT_MS); await once(installer, 'close'); - assert.deepEqual(phases, ['installing']); + assert.deepEqual(steps, ['installing']); assert.deepEqual(account.request(root, { ...request, action: 'status' }), { kind: 'failed', message: 'Wrangler installation timed out. Check your connection and try again.', @@ -135,18 +197,30 @@ test('installation timeout stops the process without starting login', async (con }); test('installation discards and bounds output', async (context) => { - const { account, installer, phases } = fixture( + const { account, installer, steps } = fixture( context, "process.stdout.write('x'.repeat(1048577)); setInterval(() => {}, 1000);", ); await once(installer, 'close'); - assert.deepEqual(phases, ['installing']); + assert.deepEqual(steps, ['installing']); assert.deepEqual(account.request(root, { ...request, action: 'status' }), { kind: 'failed', message: 'Wrangler installation returned too much output.', }); }); +test('each package manager adds the latest Wrangler as a development dependency', () => { + assert.deepEqual(wranglerInstallArguments('npm'), ['install', '--save-dev', 'wrangler']); + assert.deepEqual(wranglerInstallArguments('pnpm'), [ + 'add', + '--save-dev', + '--config.strict-dep-builds=false', + 'wrangler', + ]); + assert.deepEqual(wranglerInstallArguments('yarn'), ['add', '--dev', 'wrangler']); + assert.deepEqual(wranglerInstallArguments('bun'), ['add', '--dev', 'wrangler']); +}); + test('Wrangler detection supports project and workspace dependencies', async (context) => { const directory = await mkdtemp(path.join(tmpdir(), 'stacki-account-install-')); context.after(() => rm(directory, { recursive: true, force: true })); @@ -158,9 +232,22 @@ test('Wrangler detection supports project and workspace dependencies', async (co assert.equal(projectHasWrangler(path.join(directory, 'packages/site')), true); }); -test('default launch installs Wrangler in the requested site', async (context) => { +test('Wrangler removed while Stacki runs is detected as missing again', async (context) => { + const directory = await mkdtemp(path.join(tmpdir(), 'stacki-account-removed-')); + context.after(() => rm(directory, { recursive: true, force: true })); + const dependency = path.join(directory, 'node_modules/wrangler'); + await mkdir(dependency, { recursive: true }); + await writeFile(path.join(dependency, 'package.json'), '{"name":"wrangler"}'); + assert.equal(projectHasWrangler(directory), true); + // Node's resolution cache still returns this path after removal, as after `pnpm remove`. + await rm(dependency, { recursive: true, force: true }); + assert.equal(projectHasWrangler(directory), false); +}); + +test('default launch installs Wrangler with the site package manager', async (context) => { const directory = await mkdtemp(path.join(tmpdir(), 'stacki-account-command-')); context.after(() => rm(directory, { recursive: true, force: true })); + await writeFile(path.join(directory, 'package.json'), '{"packageManager":"npm@10.9.0"}'); setToolPathHome(homedir()); const child = spawn(process.execPath, ['-e', 'process.exitCode = 1;'], { stdio: 'pipe', @@ -171,7 +258,8 @@ test('default launch installs Wrangler in the requested site', async (context) = 'spawn', (executable: string, argumentsList: readonly string[], options: SpawnOptionsWithoutStdio) => { assert.equal(typeof executable, 'string'); - assert.deepEqual(argumentsList.slice(-3), ['install', '--save-dev', 'wrangler']); + const expected = wranglerInstallArguments('npm'); + assert.deepEqual(argumentsList.slice(-expected.length), expected); assert.equal(options.cwd, directory); assert.equal(options.shell, false); assert.equal(options.detached, process.platform !== 'win32'); diff --git a/test/renderer/publishing/publishAccountInstall.test.js b/test/renderer/publishing/publishAccountInstall.test.js index 533bc6d3..c985b184 100644 --- a/test/renderer/publishing/publishAccountInstall.test.js +++ b/test/renderer/publishing/publishAccountInstall.test.js @@ -1,5 +1,8 @@ -// Goal: installation remains visible and polling continues into browser sign-in. -// Method: render the account dialog with controlled IPC replies and advance the real poll once. +// Goal: Cloudflare sign-in stays on one screen from installation to browser sign-in or failure: +// the button waits disabled while Wrangler installs, then opens the browser, and a failure turns +// it into Try again. After an install the screen says the site's dependencies changed. +// Method: render the account dialog with controlled IPC replies and advance the real poll with +// mocked timers. const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); @@ -10,7 +13,9 @@ const { ROOT, repoPath } = require('../../helpers/sources'); const React = require('react'); const { act } = React; -test('installation keeps polling and cancellation keeps its session', async (context) => { +const browserURL = 'https://dash.cloudflare.com/oauth2/auth?state=test'; + +function loadDialog() { const output = repoPath('node_modules/.stacki-test/publish-account-install.cjs'); fs.mkdirSync(path.dirname(output), { recursive: true }); buildSync({ @@ -24,7 +29,11 @@ test('installation keeps polling and cancellation keeps its session', async (con external: ['react', 'react-dom', 'react/jsx-runtime'], logLevel: 'silent', }); - const { AddPublishAccount } = require(output); + return require(output).AddPublishAccount; +} + +async function render(context, reply) { + const AddPublishAccount = loadDialog(); const dom = new JSDOM('
', { pretendToBeVisual: true }); require('../../helpers/popupDom').installPopupDOM(dom.window); global.window = dom.window; @@ -35,9 +44,7 @@ test('installation keeps polling and cancellation keeps its session', async (con window.avb = { publishingAccount: async ({ request }) => { calls.push(request); - return request.action === 'start' - ? { kind: 'installing' } - : { kind: 'browser', url: 'https://dash.cloudflare.com/oauth2/auth?state=test' }; + return reply(request); }, }; const root = require('react-dom/client').createRoot(document.getElementById('root')); @@ -53,13 +60,26 @@ test('installation keeps polling and cancellation keeps its session', async (con }), ), ); - assert.match(document.body.textContent, /Installing Wrangler in this project/); - assert.deepEqual( - calls.map((call) => call.action), - ['start'], + return { calls, root }; +} + +function button(label) { + return [...document.querySelectorAll('button')].find((item) => item.textContent === label); +} + +test('installation and browser sign-in share one screen', async (context) => { + const { calls, root } = await render(context, (request) => + request.action === 'start' ? { kind: 'installing' } : { kind: 'browser', url: browserURL }, ); + assert.match(document.body.textContent, /Sign in to Cloudflare/); + assert.match(document.body.textContent, /Installing Wrangler in this site/); + assert.match(document.body.textContent, /lets the installation finish/); + assert.equal(button('Continue in browser').disabled, true); + assert.doesNotMatch(document.body.textContent, /Wrangler was added/); await act(async () => context.mock.timers.tick(1000)); - assert.match(document.body.textContent, /Continue in browser/); + assert.equal(button('Continue in browser').disabled, false); + assert.match(document.body.textContent, /Wrangler was added to this site/); + assert.match(document.body.textContent, /Waiting for sign-in/); assert.deepEqual( calls.map((call) => call.action), ['start', 'status'], @@ -68,3 +88,28 @@ test('installation keeps polling and cancellation keeps its session', async (con assert.equal(calls.at(-1).action, 'cancel'); assert.equal(new Set(calls.map((call) => call.id)).size, 1); }); + +test('a site that already has Wrangler says nothing about dependencies', async (context) => { + await render(context, () => ({ kind: 'browser', url: browserURL })); + assert.equal(button('Continue in browser').disabled, false); + assert.doesNotMatch(document.body.textContent, /Wrangler was added/); +}); + +test('a failure stays on the same screen and offers Try again', async (context) => { + const { calls } = await render(context, (request) => + calls.length === 1 + ? { kind: 'browser', url: browserURL } + : request.action === 'status' + ? { kind: 'failed', message: 'Cloudflare sign-in expired. Please try again.' } + : { kind: 'browser', url: browserURL }, + ); + await act(async () => context.mock.timers.tick(1000)); + assert.match(document.body.textContent, /Sign in to Cloudflare/); + assert.match(document.body.textContent, /Sign-in did not finish/); + assert.equal(button('Continue in browser'), undefined); + await act(async () => button('Try again').click()); + assert.equal(button('Continue in browser').disabled, false); + const starts = calls.filter((call) => call.action === 'start'); + assert.equal(starts.length, 2); + assert.notEqual(starts[0].id, starts[1].id); +}); From b7a1e990796307d26d145e5c432c1f2aab5e0975 Mon Sep 17 00:00:00 2001 From: SylvainDruaux Date: Fri, 23 Oct 2026 11:50:08 +0200 Subject: [PATCH 4/5] refactor(ipc): remove the unused dependency install channels Opening and creating projects install dependencies in the main process, so nothing called project:hasNodeModules or project:install any more. Removing them with their preload methods and renderer bridges leaves the renderer one fewer way to start an install. --- electron/handlers/projectHandlers.ts | 15 --------------- electron/preload/preloadBridge.ts | 2 -- shared/ipc/ipcPayloads.ts | 2 -- shared/ipc/ipcResults.ts | 4 ---- shared/ipc/preloadApi.ts | 2 -- src/ipc/appBridge.ts | 10 ---------- test/electron/app/closeProject.test.js | 1 - test/electron/app/mainBoundaries.test.ts | 2 +- test/helpers/appGestures.js | 1 - test/renderer/app/lazyPanels.test.js | 1 - test/renderer/app/pageNavigation.test.js | 1 - test/renderer/preview/componentPreview.test.js | 1 - test/shared/ipc/ipcPayloads.test.ts | 2 -- 13 files changed, 1 insertion(+), 43 deletions(-) diff --git a/electron/handlers/projectHandlers.ts b/electron/handlers/projectHandlers.ts index 89f82e35..d5b4b848 100644 --- a/electron/handlers/projectHandlers.ts +++ b/electron/handlers/projectHandlers.ts @@ -20,7 +20,6 @@ export function registerProjectHandlers( ): void { registerProjectOpenDialogHandlers(host); registerProjectCreateAstroHandlers(host); - registerProjectHasNodeModulesHandlers(host); } function registerProjectOpenDialogHandlers({ @@ -151,20 +150,6 @@ function registerProjectCreateAstroHandlers({ }); } -function registerProjectHasNodeModulesHandlers({ - ipcMain, - send, -}: Pick): void { - ipcMain.handle('project:hasNodeModules', async (_event, projectPath) => { - return fs.existsSync(path.join(projectPath, 'node_modules')); - }); - - ipcMain.handle('project:install', async (_event, projectPath) => { - await installDependencies({ send }, projectPath); - return { ok: true as const }; - }); -} - export async function installDependencies({ send }: Pick, directory: string) { ensureToolPath(); // npm/pnpm/yarn are Node shims — same PATH problem as astro const pm = detectPackageManager(directory); diff --git a/electron/preload/preloadBridge.ts b/electron/preload/preloadBridge.ts index 2811a03b..b76f66ce 100644 --- a/electron/preload/preloadBridge.ts +++ b/electron/preload/preloadBridge.ts @@ -53,8 +53,6 @@ function projectBridge() { createAstroProject: invoke('project:createAstro'), parentDialog: invoke('project:parentDialog'), createStarter: invoke('project:createStarter'), - hasNodeModules: invoke('project:hasNodeModules'), - installDeps: invoke('project:install'), scanProject: invoke('project:scan'), listProjectClasses: invoke('project:classes'), watchProject: invoke('watch:start'), diff --git a/shared/ipc/ipcPayloads.ts b/shared/ipc/ipcPayloads.ts index e6957958..3ce1dadd 100644 --- a/shared/ipc/ipcPayloads.ts +++ b/shared/ipc/ipcPayloads.ts @@ -270,9 +270,7 @@ export const IPC_PAYLOADS = { ai: maybeFlag, }), 'project:createStarter': object({ starter: optional(text), parentPath: pathText, name: text }), - 'project:hasNodeModules': pathText, 'project:injectedRoutes': object({ ...project }), - 'project:install': pathText, 'project:newDialog': nothing, 'project:openDialog': nothing, 'project:parentDialog': nothing, diff --git a/shared/ipc/ipcResults.ts b/shared/ipc/ipcResults.ts index 1a617a93..4e995766 100644 --- a/shared/ipc/ipcResults.ts +++ b/shared/ipc/ipcResults.ts @@ -564,13 +564,9 @@ export interface IpcResults { readonly ok: boolean; readonly projectPath: string; }; - readonly 'project:hasNodeModules': boolean; readonly 'project:injectedRoutes': { readonly routes: ReadonlyArray; }; - readonly 'project:install': { - readonly ok: true; - }; readonly 'project:newDialog': | { readonly canceled: true; diff --git a/shared/ipc/preloadApi.ts b/shared/ipc/preloadApi.ts index fc72e2d7..f4a28d87 100644 --- a/shared/ipc/preloadApi.ts +++ b/shared/ipc/preloadApi.ts @@ -15,8 +15,6 @@ interface InvokeChannels { readonly createAstroProject: 'project:createAstro'; readonly parentDialog: 'project:parentDialog'; readonly createStarter: 'project:createStarter'; - readonly hasNodeModules: 'project:hasNodeModules'; - readonly installDeps: 'project:install'; readonly scanProject: 'project:scan'; readonly listProjectClasses: 'project:classes'; readonly watchProject: 'watch:start'; diff --git a/src/ipc/appBridge.ts b/src/ipc/appBridge.ts index ee4db9ee..67de7862 100644 --- a/src/ipc/appBridge.ts +++ b/src/ipc/appBridge.ts @@ -90,11 +90,6 @@ export function readProjectClasses(projectPath: string): Promise { - const payload = parseIpcPayload('project:hasNodeModules', projectPath); - return window.avb.hasNodeModules(payload).then(boolean); -} - export function pendingProject(): Promise { return window.avb.pendingProject().then((input) => optional(pathText)(input)); } @@ -115,11 +110,6 @@ export function addRecentProject(projectPath: string): Promise { return window.avb.addRecent(payload).then((input) => void parseOkResult(input)); } -export function installProjectDependencies(projectPath: string): Promise { - const payload = parseIpcPayload('project:install', projectPath); - return window.avb.installDeps(payload).then((input) => void parseOkResult(input)); -} - export function watchProject(projectPath: string): Promise { const payload = parseIpcPayload('watch:start', projectPath); return window.avb.watchProject(payload).then((input) => boolean(record(input)['ok'])); diff --git a/test/electron/app/closeProject.test.js b/test/electron/app/closeProject.test.js index 32859654..d2f840b2 100644 --- a/test/electron/app/closeProject.test.js +++ b/test/electron/app/closeProject.test.js @@ -107,7 +107,6 @@ const settle = (ms = 30) => new Promise((resolve) => setTimeout(resolve, ms)); }, listProjectClasses: async () => [], startDevServer: async () => ({ url: 'http://localhost:4321' }), - hasNodeModules: async () => true, gitInfo: async () => ({ isRepo: false }), publishAvailability: async () => ({ github: 'unknown', hosting: 'unconfigured' }), gitLog: async () => ({ commits: [], atEnd: true }), diff --git a/test/electron/app/mainBoundaries.test.ts b/test/electron/app/mainBoundaries.test.ts index 966bda5a..28bd55d9 100644 --- a/test/electron/app/mainBoundaries.test.ts +++ b/test/electron/app/mainBoundaries.test.ts @@ -62,7 +62,7 @@ test('the complete channel inventory matches real main and terminal registration // Account sign-in adds one project-scoped channel for start, status, and cancellation. // Published-site discovery has its own channel so remote lookups cannot block local status. // Git pull and its background incoming check each have a project-scoped channel. - assert.equal(harness.handlers.size, 137); + assert.equal(harness.handlers.size, 135); assert.equal(terminalChannels.length, 4); assert.deepEqual( [...Object.keys(IPC_PAYLOADS)].sort(), diff --git a/test/helpers/appGestures.js b/test/helpers/appGestures.js index e84e6374..a1c187b4 100644 --- a/test/helpers/appGestures.js +++ b/test/helpers/appGestures.js @@ -114,7 +114,6 @@ async function mountApp(root, files, build, options = {}) { input.projectPath, input.write, ), - hasNodeModules: async () => true, startDevServer: async () => ({ url: 'http://localhost:4321' }), listProjectClasses: async () => [], readPage: async (file) => read(file), diff --git a/test/renderer/app/lazyPanels.test.js b/test/renderer/app/lazyPanels.test.js index c0d38404..603f7e8c 100644 --- a/test/renderer/app/lazyPanels.test.js +++ b/test/renderer/app/lazyPanels.test.js @@ -371,7 +371,6 @@ function createBridge() { components: [], layouts: [], }), - hasNodeModules: async () => true, startDevServer: async () => ({ url: 'http://localhost:4321' }), listProjectClasses: async () => [], // Disk replies carry the SHA-256 of the bytes, as main's do. diff --git a/test/renderer/app/pageNavigation.test.js b/test/renderer/app/pageNavigation.test.js index 66f3403a..02214e1b 100644 --- a/test/renderer/app/pageNavigation.test.js +++ b/test/renderer/app/pageNavigation.test.js @@ -141,7 +141,6 @@ test('out-of-order page reads and external reads cannot replace the current edit scans.push(request); return request.promise; }, - hasNodeModules: async () => true, startDevServer: async () => ({ url: 'http://localhost:4321' }), listProjectClasses: async () => [], readPage: (file) => { diff --git a/test/renderer/preview/componentPreview.test.js b/test/renderer/preview/componentPreview.test.js index e3e75c4d..ce2368af 100644 --- a/test/renderer/preview/componentPreview.test.js +++ b/test/renderer/preview/componentPreview.test.js @@ -138,7 +138,6 @@ test( layouts: [], pageFolders: [], }), - hasNodeModules: async () => true, startDevServer: async () => ({ url: 'http://localhost:4321' }), listProjectClasses: async () => [], resolveImport: async () => ({ path: card.path }), diff --git a/test/shared/ipc/ipcPayloads.test.ts b/test/shared/ipc/ipcPayloads.test.ts index 9d927ece..6fd14fdd 100644 --- a/test/shared/ipc/ipcPayloads.test.ts +++ b/test/shared/ipc/ipcPayloads.test.ts @@ -39,8 +39,6 @@ const pathPayload = new Set([ 'recents:add', 'recents:remove', 'recents:refreshThumb', - 'project:hasNodeModules', - 'project:install', 'project:scan', 'project:classes', 'watch:start', From 1e3885b1f3f8517e531370d51b55f564fce92806 Mon Sep 17 00:00:00 2001 From: SylvainDruaux Date: Fri, 23 Oct 2026 12:04:53 +0200 Subject: [PATCH 5/5] fix(project): don't fail pnpm installs on ignored build scripts pnpm 11 and later exit with an error when they block a dependency's build script, such as Astro's esbuild, even though everything else is installed, so opening or creating a pnpm project failed. Installs now keep those scripts blocked without failing, as the Wrangler install does. The missing-manager error no longer claims a lockfile was found, since the manager can also come from package.json. --- electron/handlers/projectHandlers.ts | 8 +++----- electron/project/packageManager.ts | 21 ++++++++++++++++++++ electron/project/publishAccount.ts | 6 ++---- test/electron/project/packageManager.test.ts | 14 ++++++++++--- 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/electron/handlers/projectHandlers.ts b/electron/handlers/projectHandlers.ts index d5b4b848..4ed08262 100644 --- a/electron/handlers/projectHandlers.ts +++ b/electron/handlers/projectHandlers.ts @@ -12,7 +12,7 @@ import { createStarter } from '../project/starter'; import { isWin, errorMessage } from '../lib/mainHelpers'; import { ensureToolPath, run } from '../lib/nodeTools'; import { isAstroProject } from '../project/projectFiles'; -import { detectPackageManager } from '../project/packageManager'; +import { detectPackageManager, installArguments } from '../project/packageManager'; import type { MainHost } from './mainHost'; export function registerProjectHandlers( @@ -154,17 +154,15 @@ export async function installDependencies({ send }: Pick, dire ensureToolPath(); // npm/pnpm/yarn are Node shims — same PATH problem as astro const pm = detectPackageManager(directory); send('progress', { message: `Installing dependencies (${pm} install)…` }); - const args = pm === 'npm' ? ['install', '--no-audit', '--no-fund'] : ['install']; try { - await run(isWin ? `${pm}.cmd` : pm, args, directory, { + await run(isWin ? `${pm}.cmd` : pm, [...installArguments(pm)], directory, { timeout: 10 * 60 * 1000, shell: isWin, }); } catch (error: unknown) { if (toRecord(error)?.['code'] === 'ENOENT') { throw new Error( - `This project uses ${pm} (found its lockfile), but ${pm} is not installed. ` + - 'Install it and try again.', + `This project uses ${pm}, but ${pm} is not installed. Install it and try again.`, ); } throw new Error(`${pm} install failed: ${gitErrorDetail(error).slice(-400)}`); diff --git a/electron/project/packageManager.ts b/electron/project/packageManager.ts index 57fd6efa..5f4d6668 100644 --- a/electron/project/packageManager.ts +++ b/electron/project/packageManager.ts @@ -9,6 +9,27 @@ import { type PackageManager, } from '../../shared/properties/packageManager'; +// By default `pnpm` blocks dependency build scripts and, since v11, exits with an error when it +// does, though everything else is installed. Keep them blocked; just don't fail the install. +export const PNPM_TOLERATE_IGNORED_BUILDS = '--config.strict-dep-builds=false'; + +// Installs a project's dependencies with its own manager. +export function installArguments(manager: PackageManager): readonly string[] { + switch (manager) { + case 'npm': + return ['install', '--no-audit', '--no-fund']; + case 'pnpm': + return ['install', PNPM_TOLERATE_IGNORED_BUILDS]; + case 'yarn': + case 'bun': + return ['install']; + default: { + const exhaustive: never = manager; + return exhaustive; + } + } +} + // The packageManager field in package.json names it outright; otherwise the // lockfile shows which one last installed; else npm. export function detectPackageManager(directory: string): PackageManager { diff --git a/electron/project/publishAccount.ts b/electron/project/publishAccount.ts index 27f2eb97..491cc456 100644 --- a/electron/project/publishAccount.ts +++ b/electron/project/publishAccount.ts @@ -11,7 +11,7 @@ import { } from '../../shared/ipc/publishAccount'; import { packageManagerLabel, type PackageManager } from '../../shared/properties/packageManager'; import { ensureToolPath } from '../lib/nodeTools'; -import { detectPackageManager } from './packageManager'; +import { detectPackageManager, PNPM_TOLERATE_IGNORED_BUILDS } from './packageManager'; import { projectHasWrangler, resolvePublishExecutable } from './publishExecutable'; import { publishEnvironment, stopPublishProcess } from './publishCommands'; @@ -300,9 +300,7 @@ export function wranglerInstallArguments(manager: PackageManager): readonly stri case 'npm': return ['install', '--save-dev', 'wrangler']; case 'pnpm': - // By default `pnpm` blocks dependency build scripts and, since v11, exits with an error - // when it does, though Wrangler is installed. Keep them blocked; just don't fail. - return ['add', '--save-dev', '--config.strict-dep-builds=false', 'wrangler']; + return ['add', '--save-dev', PNPM_TOLERATE_IGNORED_BUILDS, 'wrangler']; case 'yarn': case 'bun': return ['add', '--dev', 'wrangler']; diff --git a/test/electron/project/packageManager.test.ts b/test/electron/project/packageManager.test.ts index 9d2fd6df..a1ad8d3a 100644 --- a/test/electron/project/packageManager.test.ts +++ b/test/electron/project/packageManager.test.ts @@ -1,13 +1,14 @@ // Goal: a project's package manager comes from package.json's packageManager field when it -// names one Stacki runs, then from its lockfile, then npm. +// names one Stacki runs, then from its lockfile, then npm; each installs with its own command. // Method: parse valid, versioned, unknown, malformed and oversized fields, then detect inside -// temporary project folders that combine fields and lockfiles. Nothing is installed. +// temporary project folders that combine fields and lockfiles, and pin each install command. +// Nothing is installed. import assert from 'node:assert/strict'; import test, { type TestContext } from 'node:test'; import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import path from 'node:path'; -import { detectPackageManager } from '#dist/electron/project/packageManager.js'; +import { detectPackageManager, installArguments } from '#dist/electron/project/packageManager.js'; import { PACKAGE_MANAGERS, packageManagerLabel, @@ -45,6 +46,13 @@ test('the packageManager field names nothing when unknown, malformed or oversize } }); +test('each package manager installs dependencies with its own command', () => { + assert.deepEqual(installArguments('npm'), ['install', '--no-audit', '--no-fund']); + assert.deepEqual(installArguments('pnpm'), ['install', '--config.strict-dep-builds=false']); + assert.deepEqual(installArguments('yarn'), ['install']); + assert.deepEqual(installArguments('bun'), ['install']); +}); + test('every package manager has the name people know it by', () => { assert.deepEqual( PACKAGE_MANAGERS.map((manager) => packageManagerLabel(manager)),