|
| 1 | +# W-23072534 — TerminalService.simpleExec object param + Duration timeout |
| 2 | + |
| 3 | +## Context |
| 4 | +- `simpleExec(command, parse?, timeoutMs?)` → positional. Replace w/ single options object. |
| 5 | +- New sig: `simpleExec({ command, parse?, timeout? })`; `timeout: Duration.DurationInput`, default `Duration.millis(30_000)`; internally `Duration.toMillis`. |
| 6 | +- Callers: orgDelete.ts (timeout 120s), projectInfo.ts (command-only x2). |
| 7 | +- Test: orgDelete.test.ts assertions on positional args (lines 58, 67, 75-79). |
| 8 | +- Duration import convention: `import * as Duration from 'effect/Duration'`. |
| 9 | + |
| 10 | +## External-consumers (blast radius) |
| 11 | +`TerminalService` is on the public `SalesforceVSCodeServicesApi` surface (index.ts:142, `export type` at index.ts:479). Changing `simpleExec`'s signature is a breaking API change for external callers. Checked: |
| 12 | +- monorepo grep `simpleExec`: only callers are orgDelete.ts, projectInfo.ts, orgDelete.test.ts (all in-repo). |
| 13 | +- `gh` search `simpleExec org:forcedotcom` + `org:salesforcecli`: only this monorepo + its mirror repos (salesforcedx-vscode-ci-testing, vibes-poc) + `code-analyzer-core` `simpleExecutor` (unrelated class property, not TerminalService). |
| 14 | +- No external consumer calls `TerminalService.simpleExec`. |
| 15 | + |
| 16 | +**Decision:** breaking change is safe — no external consumer. No compatibility overload needed. Re-run the external-consumers check at build time (`gh` search may have new hits) before committing; if a new consumer appears, add an object/positional overload or document the break. |
| 17 | + |
| 18 | +## Phases |
| 19 | + |
| 20 | +### Phase 1 — source signature + non-test callers |
| 21 | +Change source only; do NOT touch the test yet (so tsc/jest break loudly on the stale positional mock assertions, proving the test catches the new shape). |
| 22 | + |
| 23 | +**Files:** |
| 24 | +- `packages/salesforcedx-vscode-services/src/terminal/terminalService.ts` |
| 25 | + - sig → `simpleExec: ({ command, parse = s => s, timeout = Duration.millis(30_000) }: { command: string; parse?: (stdout: string) => string; timeout?: Duration.DurationInput })` |
| 26 | + - `exec(command, { timeout: Duration.toMillis(timeout) })` |
| 27 | + - add `import * as Duration from 'effect/Duration'`; update jsdoc (`timeout` not `timeoutMs`) |
| 28 | +- `packages/salesforcedx-vscode-org/src/commands/orgDelete.ts` |
| 29 | + - `DELETE_TIMEOUT_MS = 120_000` → `DELETE_TIMEOUT = Duration.seconds(120)`; update comment (line 140) |
| 30 | + - call (line 173) → `simpleExec({ command: \`sf ${deleteSubcommand}${targetOrgFlag} --no-prompt\`, parse: identity, timeout: DELETE_TIMEOUT })` |
| 31 | + - add Duration import |
| 32 | +- `packages/salesforcedx-vscode-metadata/src/commands/projectInfo.ts` |
| 33 | + - `simpleExec({ command: 'sf --version' })`, `simpleExec({ command: 'java --version' })` (lines 149-150) |
| 34 | + |
| 35 | +**Gate:** `tsc` org+services+metadata compile must PASS (source consistent). Run jest orgDelete.test.ts and confirm it FAILS on the stale positional `toHaveBeenCalledWith` — this proves the assertions are load-bearing and not false-green. Note: positional mock-arg assertions are JS-level, not type-checked, so tsc alone won't flag them; the jest run is the guard. |
| 36 | + |
| 37 | +### Phase 2 — test assertions to object shape |
| 38 | +**Files:** |
| 39 | +- `packages/salesforcedx-vscode-org/test/jest/commands/orgDelete.test.ts` |
| 40 | + - 3 `toHaveBeenCalledWith` (lines 58, 67, 75-79) → single object arg: `{ command: '...', parse: expect.any(Function), timeout: Duration.seconds(120) }` |
| 41 | + - add `import * as Duration from 'effect/Duration'` |
| 42 | + - `Duration.seconds(120)` is a structural value; jest deep-equality matches it. Verify the assertion fails if `timeout` is omitted/wrong (sanity: the Phase-1 red proves lock-step). |
| 43 | + |
| 44 | +**Gate:** jest orgDelete.test.ts now PASSES against new shape. |
| 45 | + |
| 46 | +**Commit (after both phases):** `refactor(services): simpleExec object param w/ Duration timeout - W-23072534` |
| 47 | + |
| 48 | +## Skills |
| 49 | +- external-consumers (public API surface — breaking change blast radius) |
| 50 | +- effect-best-practices (Duration usage) |
| 51 | +- services-extension-consumption (API surface change consumed by org/metadata) |
| 52 | +- typescript |
| 53 | +- playwright-e2e (projectInfo spec exercises the changed path) |
| 54 | +- concise |
| 55 | +- verification |
| 56 | + |
| 57 | +## Verification |
| 58 | +- `simpleExec` grep: no remaining positional callers |
| 59 | +- compile: services, org, metadata packages |
| 60 | +- jest: orgDelete.test.ts (object-shape assertions; `Duration.seconds(120)` deep-equality) |
| 61 | +- lint changed files |
| 62 | +- re-run external-consumers `gh` search before commit (no new external caller) |
| 63 | + |
| 64 | +### E2E |
| 65 | +Two existing specs touch the changed packages: |
| 66 | +- `packages/salesforcedx-vscode-org/test/playwright/specs/orgDeleteCommandVisibility.desktop.spec.ts` — checks command *visibility* in the palette only; does NOT invoke delete, so does NOT exercise `simpleExec`. The exec call path is jest-covered (orgDelete.test.ts). Not required for this change, but run to confirm no regression in org desktop bundle. |
| 67 | +- `packages/salesforcedx-vscode-metadata/test/playwright/specs/projectInfo.headless.spec.ts` — runs "Generate Project Info", which calls `projectInfo.ts` → `simpleExec({ command: 'sf --version' })` / `simpleExec({ command: 'java --version' })`; the `## Environment` section assertion (spec line 76) depends on those exec results. **DOES exercise the new object-param call path. Required.** |
| 68 | + |
| 69 | +Run before PR (from repo root, no `cd`): |
| 70 | +- `npm run test:desktop -w salesforcedx-vscode-metadata -- --retries 0` (projectInfo.headless is desktop-gated via `isDesktop()`) |
| 71 | +- `npm run test:desktop -w salesforcedx-vscode-org -- --retries 0` (regression check on visibility spec) |
| 72 | + |
| 73 | +If projectInfo spec cannot run locally (no org/sf/java), document the blocker; the jest+compile gates plus the spec's `## Environment` dependency on simpleExec remain the evidence the path is wired correctly. |
0 commit comments