diff --git a/.agents/specs/2026-07-29-typed-client-contract-binding-design.md b/.agents/specs/2026-07-29-typed-client-contract-binding-design.md new file mode 100644 index 00000000..fd78af06 --- /dev/null +++ b/.agents/specs/2026-07-29-typed-client-contract-binding-design.md @@ -0,0 +1,271 @@ +# Design: decouple the client from the contract + +- **Date:** 2026-07-29 +- **Status:** approved, not yet implemented +- **Target:** `@temporal-contract/client`, 8.0 beta line +- **Origin:** [millenium!56319 note 2210871](https://gitlab.factory.fonciamillenium.net/FonciaStark/millenium/-/merge_requests/56319#note_2210871) + +> Design docs live in `.agents/specs/`, not `docs/`. The VitePress config sets no +> `srcExclude`, so every `.md` under `docs/` becomes a published page. + +## Context + +The originating comment asks two things: + +> maybe we could create a nestjs provider for the TypedClient +> +> does it make sense to give the contract when calling `TypedClient.create`? It +> means we must have one TypedClient per contract. Instead we could have only one +> TypedClient and give the contract when calling `startWorkflow` or +> `executeWorkflow`? Or maybe we could find an other better DX + +It sits on `lessor-tax-declaration.service.ts` in a draft MR titled _"POC: evaluate +replacing the in-house Temporal integration with temporal-contract (compute +slice)"_, where a `TypedClient` is constructed **inside a request-scoped method**: + +```ts +async refreshDeclaration(declarationId: string): Promise { + // ... + const typedClient = await TypedClient.create({ + contract: leaseAccountingComputeContract, + client: this.client, + }) + .mapErr((error) => match(error).with(tag("@temporal-contract/TechnicalError"), /* ... */).exhaustive()) + .getOrThrow(); + + await typedClient.startWorkflow("computeOneLessorTaxDeclarationWorkflow", { /* ... */ }); +} +``` + +### What the investigation found + +1. **Construction is fallible and async only because of the connection.** `create` + awaits `ensureConnected()` and routes setup faults to the defect channel. + Contract binding itself is synchronous and free — `contract` feeds only + `taskQueue`, workflow lookup, schema validation, and `TypedScheduleClient`. + The two concerns are fused, which is what pushes construction into a request + handler. +2. **Part of the pain is a version artifact.** The consumer pins + `catalog:temporalContract7`. On 8.0 `create` returns + `AsyncResult<..., never>`, so the `mapErr` / + `tag("@temporal-contract/TechnicalError")` ceremony above does not compile and + collapses to `.get()`. +3. **The multi-contract case is not yet present.** Across the millenium monorepo, + temporal-contract is consumed by `applications/plato` (one contract) and, in + this POC, `service-lease-accounting` (`leaseAccountingComputeContract`). One + contract per application; no process binds two today. The driver for this + change is therefore separation of concerns, not contract count — see Rationale. +4. **Precedent exists for contract-per-call.** The worker's + `context.startChildWorkflow(contract, name, options)` takes the contract as a + required first parameter. + +## Rationale + +A client is a **connection**. A contract is a **schema**. Coupling them means a +connection cannot be established without first choosing a schema, which is +backwards: the connection is the scarce, fallible, process-lifetime resource, and +the schema is a free compile-time artifact. + +The practical consequences of the current coupling: + +- Establishing a connection requires naming a contract, so an application with + two contracts opens two clients and runs `ensureConnected()` twice. +- The fallible, async step cannot be hoisted to process start without also fixing + the contract there, which is what pushed construction into a request handler in + the originating MR. + +## Non-goals + +- **A NestJS integration package.** `@temporal-contract/client-nestjs` and + `@temporal-contract/worker-nestjs` were deliberately removed in PR #116 + (`53dfb80`, 2026-03-01) to "simplify the project scope and reduce maintenance + burden". The consumer additionally has its own in-house `@emeria/nestjs-temporal` + (`@ContractActivitiesHandler()`, activity explorer), so the need is already met + outside this repo. The removed provider was also one-module-per-contract, so it + would not have addressed this complaint anyway. +- **Moving the contract onto every call** + (`client.startWorkflow(contract, name, options)`). Rejected: it taxes every + call site, and `client.schedule` would need the contract threaded through + `TypedScheduleClient` as well. Binding once via `for()` gives the same + decoupling without the per-call cost. +- **Applying the same decoupling to the worker.** The client's contract coupling + was accidental — a connection and a schema have nothing to do with each other. + The worker's is essential: a Temporal `Worker` instance polls exactly one task + queue, and the contract is the worker's job description — it supplies the task + queue (`packages/worker/src/worker.ts` passes `taskQueue: contract.taskQueue`), + the workflows and activities to register, and the schemas to validate against. + An unbound worker would have nothing to poll and nothing to run, so + `createWorker({ contract, ... })` stays as-is. (The invariant is one `Worker` + _instance_ per task queue, not one worker process — Temporal scales + horizontally by running many processes on the same queue.) The client-side + principle — share the scarce connection — already holds on the worker side + without an API change: `createWorker` spreads its remaining options through to + `Worker.create`, so a process hosting two contracts runs two `Worker` instances + sharing one `NativeConnection`. The only gap is two contracts declaring the + _same_ `taskQueue` string, which would need one worker registering both + contracts' handlers; that is a design smell (two schema universes interleaved + on one queue) and stays unsupported until a real need appears. + +## Decision + +Split the class in two. + +```ts +/** Connection-scoped. No contract, no type parameter. */ +export class TypedClient { + static create(options: { + client: Client; + interceptors?: readonly ClientInterceptor[]; + }): AsyncResult; + + /** Bind a contract. Synchronous, infallible, memoized per contract identity. */ + for(contract: TContract): ContractClient; +} + +/** Contract-scoped. Everything TypedClient exposes today. */ +export class ContractClient { + startWorkflow(...): ...; + executeWorkflow(...): ...; + signalWithStart(...): ...; + getHandle(...): ...; + readonly schedule: TypedScheduleClient; +} +``` + +```ts +const client = await TypedClient.create({ client: temporal }).get(); // once, at startup + +await client.for(leaseContract).startWorkflow("computeOneLessorTaxDeclarationWorkflow", { + workflowId, + args, +}); +await client.for(platoContract).startWorkflow("someOtherWorkflow", { workflowId, args }); +``` + +No contract is privileged; every contract is reached the same way. + +### Naming + +`TypedClient` keeps its name even though the type parameter moves to +`ContractClient`. Renaming the root (`TemporalClient`, `ClientRoot`, …) was +considered — the unshipped beta is the only cheap window — and rejected: +`TypedClient` is the package's documented entry point, every consumer and doc +starts from it, and the change is easier to teach as "`TypedClient` now hands +out contract-bound clients" than as two renames at once. The name stays honest +enough: the root is what makes the client typed, via `for()`. + +### Semantics + +- **`for()` is infallible.** The `@temporalio/client >= 1.16` check (a missing + `client.schedule`) moves to `TypedClient.create`, where it belongs — it is a + property of the connection, not of any contract. `for()` therefore returns + `ContractClient` directly, with no `AsyncResult` wrapper. This is the + ergonomic win: binding is a plain expression, valid in a field initializer. +- **Memoized and identity-stable.** A + `WeakMap>` on the root, so + repeated `for(c)` returns the same instance rather than rebuilding + `TypedScheduleClient`. The map erases the type parameter, so the store and the + read each require a cast, contained to the two lines inside `for()`. Dropping + memoization was considered (construction is cheap, and the casts would go with + it) but identity stability keeps `for()` free to call in hot paths — the + natural consumer shape is `this.client.for(contract).startWorkflow(...)` per + request — without allocating a `TypedScheduleClient` per call. Note the + constraint this places on a future `for(contract, options)` overload: only the + option-less call could serve from the memo, so the `for(c) === for(c)` + guarantee is documented for the option-less form only. +- **`ContractClient` inherits `client` and `interceptors` from the root.** No + per-call interceptor override — YAGNI, and addable later without a break. +- **`create` still awaits `ensureConnected()`**, once per process rather than once + per contract. + +## Breaking changes + +8.0 has not shipped, so these land inside the existing beta line rather than +forcing a new major. + +| Before | After | +| -------------------------------------------------- | ---------------------------------------------- | +| `TypedClient.create({ contract, client })` | `TypedClient.create({ client }).for(contract)` | +| `TypedClient` (type annotation) | `ContractClient` | +| `TypedClient.createOrThrow(contract, client, ...)` | removed | + +`create({ contract, client })` is **not** retained as a deprecated alias: keeping +it would preserve exactly the contract dependency this change removes. +`createOrThrow` is already `@deprecated` and takes a contract positionally, so it +goes at the same time. + +Roughly 15 references to `TypedClient<...>` exist in-repo (tests, examples, +`docs/reference/client-surface.md`); about 6 are `TypedClient` +annotations that become `ContractClient`. + +## Implementation + +`packages/client/src/client.ts`. + +1. Rename `class TypedClient` to `ContractClient`, keeping + every member as-is. Its constructor stays private. +2. Move the `client.schedule` presence check out of that constructor into the new + root's `create`. +3. Add `class TypedClient` holding `client`, `interceptors`, and the memo + `WeakMap`; `static create({ client, interceptors })` performs the schedule check + and `ensureConnected()`, returning `AsyncResult`. +4. Add `TypedClient#for(contract)`: read the memo, otherwise construct a + `ContractClient` with `(contract, this.client, this.interceptors)`, store, + return. +5. Delete `createOrThrow` and `CreateTypedClientOptions`'s `contract` field. +6. Export `ContractClient` from the package entry point. +7. Add a TSDoc `@example` to both `create` and `for`, each showing its imports — + per the convention that a standalone example block names where its symbols come + from. + +`ContractClient` must be constructible from `TypedClient#for`. Since they are +separate classes, the constructor cannot stay `private`; make it `internal` by +convention (a documented `@internal` TSDoc tag plus omission from the public +surface docs) rather than exporting a factory. + +> `client.ts` is already ~1100 lines. Splitting `ContractClient` into its own +> module is tempting but would balloon the diff and obscure the rename in review. +> Keep both classes in `client.ts` for this change; split separately if it grows. + +## Testing + +| Level | File | Cases | +| ----------- | --------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Type | `packages/client/src/types-inference.spec.ts` | `for(c).startWorkflow(name)` accepts only `c`'s workflow names; two different contracts yield mutually incompatible `ContractClient` types; `TypedClient` accepts no type argument, so a stray 7.x-style `TypedClient` fails loudly during migration rather than resolving silently. This is where the real risk lives. | +| Unit | `packages/client/src/client.spec.ts` | `for(c)` twice returns the same instance; two contracts yield distinct instances; each uses its own `taskQueue` and validates against its own schemas; interceptors are inherited; `ensureConnected()` runs once per root; `create` rejects a `Client` without `schedule`. | +| Integration | `packages/client/src/__tests__/` | New `second.contract.ts` + `second.workflows.ts` fixtures and a second `Worker` on the second task queue; one case proving one root drives both contracts and that `for(secondContract).executeWorkflow(...)` is routed to the second queue against a live server. | + +Existing suites need their `TypedClient` fixtures retyped to +`ContractClient` and their construction updated to +`create({ client }).for(contract)`. The integration fixture mirrors the existing +`worker` vitest fixture in `__tests__/client.spec.ts` (`Worker.create` with +`taskQueue`, `workflowsPath`, `{ auto: true }`, shutdown in teardown). + +## Documentation + +- `docs/reference/client-surface.md` — document both classes and `for()`. +- `docs/how-to/upgrade-to-v8.md` — a migration section for the three breaking + changes in the table above. +- Every `TypedClient.create({ contract, client })` occurrence across `docs/`, + `README.md`, `packages/*/README.md` and `examples/` updated to the two-step + form. +- TSDoc `@example` on `create` and `for`, which render into the generated API docs. + +## Release + +A `major` changeset on `@temporal-contract/client` — the change is breaking. The +repo is in changesets pre mode on the `beta` tag, so it folds into the next +`8.0.0-beta.N` rather than opening a 9.0 line; the fixed group bumps all four +packages together. + +## Unrelated defect found while investigating + +`packages/worker/src/workflow.ts` — the TSDoc for `startChildWorkflow` and +`executeChildWorkflow` claims: + +> - Same contract: Pass workflowName from current contract +> - Cross-contract: Pass contract and workflowName to invoke workflows from other workers + +Both signatures take `contract` as a **required** first parameter; there is no +same-contract overload. The documentation describes an API that does not exist. +Fix separately.