diff --git a/docs/project/readable-codebase/RD-05-store-effect-map.md b/docs/project/readable-codebase/RD-05-store-effect-map.md new file mode 100644 index 0000000..cae74aa --- /dev/null +++ b/docs/project/readable-codebase/RD-05-store-effect-map.md @@ -0,0 +1,182 @@ +# RD-05 — `createStore` gains an effect map + +Status: done +Source: PLAN.md 1a + +## Why + +`runIfSubmitting` is not shared. It is a `private async` method copy-pasted into 5 components +under 2 names, and it must be called by hand immediately after `dispatch`: + +```ts +this.dispatch({ tag: 'Submit' }); // the reducer decides +this.runIfSubmitting(); // then re-read state() and re-check the tag it hoped for +``` + +**Forgetting the second line fails silently.** This ticket makes that impossible by moving the +effect into the one sanctioned wiring idiom, so entering a state runs its effect by +construction. + +This ticket is **behaviour-neutral**: it changes no call site. RD-06 and RD-08 migrate them. +Landing the mechanism alone keeps the risk isolated to one reviewable commit. + +## Read first + +- `libs/shared/src/application/store.ts` — all 33 lines. The comment at lines 27-30 is the + specification for the invocation order below. +- `libs/shared/src/application/store.spec.ts` — both existing tests. **Neither may be + deleted or weakened.** +- `PLAN.md` 1a, including the two rejected alternatives. +- `libs/shared/src/application/submit.ts` — `runResult`, `runSubmit`, `SUBMIT_FAILED`. + +## Decisions (pre-made, don't relitigate) + +1. **The API.** In `libs/shared/src/application/store.ts`: + + ```ts + export type StoreEffects = Model extends { tag: string } + ? { + [K in Model['tag']]?: ( + state: Extract, + store: Store, + ) => unknown; + } + : never; + + export function createStore( + init: Model, + update: (model: Model, msg: Msg) => Model, + effects?: StoreEffects, + ): Store; + ``` + +2. **A conditional type, NOT a `Model extends { tag: string }` constraint.** This is not + stylistic. `store.spec.ts:8` calls `createStore(0, (n: number, m: number) => n + m)`, where + `Model = number` and has no `tag`. A constraint breaks that existing spec. With the + conditional, `Model = number` resolves `StoreEffects` to `never`, so passing effects there + is a compile error while omitting them stays legal. + +3. **Keys are `Model['tag']`**, so a renamed or misspelled state tag is a compile error. This + buys the typo half of exhaustiveness. The completeness half (a full Elm `[state, Cmd]`) is + knowingly not bought — see PLAN.md 1a for why. + +4. **The narrowed state is argument one.** This is what deletes the + `const s = this.state(); if (s.tag !== 'Submitting') return;` preamble at all 8 call sites + in RD-06/RD-08. The body cannot run in the wrong state, so it cannot re-guess it. + +5. **The store is argument two.** The effect needs `dispatch`, but `createStore(...)` runs in + a field initializer before `this.store` is assigned. Build the store object, then close + over it, so the effect is independent of field-declaration order. + +6. **The trigger rule.** Run `effects[next.tag]` when **both** hold: + - `prev.tag !== next.tag` — the store _entered_ the tag. A `Submit` that fails validation + is `Editing → Editing`: no fire. A second `Submit` while `Submitting` is a reducer + no-op: no fire, so **double-submit protection falls out of the rule**. `Retry` is + `Failed → Submitting`: fires, so `onRetry` needs no special case anywhere. + - **the msg tag is not `Seed`.** Without this, the five components that mount a + `Submitting` state in Storybook via `Seed` fire real network calls (see Risks), and + `draftSync.onResume` re-submits a resumed draft. Document it on `StoreEffects` as the + convention it already is: `Seed` is the mount/restore message in all 7 machines that + have one. + +7. **Invocation order.** Capture `prev` and `next` inside the `model.update(...)` callback + into locals, and invoke the effect **after** `update` returns. Do **not** read `model()` + inside `dispatch`, and do **not** invoke the effect inside the updater (a signal write + nested in an updater). `store.ts:27-30` explains why: a tracked read there makes an + effect depend on its own write and livelocks the main thread. It already crashed the + upload wizards once. + +8. **The tag check must be safe on a non-object `Msg`.** Same reason as decision 2 — + `store.spec.ts:8` dispatches plain numbers. Guard with + `typeof msg === 'object' && msg !== null && 'tag' in msg`, never a bare `msg.tag`. The + same applies to reading `prev.tag`/`next.tag` when `Model` is not an object. + +9. **`dispatch` stays `void`-returning.** The effect's promise is floated, exactly as + `this.runIfSubmitting();` is floated today. Every effect body ends in a `Result` from + `runSubmit`/`runResult`, so it cannot throw — state that as the effect contract in the doc + comment rather than adding a try/catch. + +10. **No call site changes in this ticket.** Do not migrate any component. Do not touch the + 5 components or any `*.machine.ts`. + +## Files + +- `libs/shared/src/application/store.ts` — the type, the third parameter, the trigger rule, + and a doc comment covering the effect contract and the `Seed` convention. +- `libs/shared/src/application/store.spec.ts` — 5 new cases, both existing cases untouched. + +## Steps + +1. Add `StoreEffects` per decision 1. +2. Add the optional third parameter and implement the trigger rule per decisions 6-8. +3. Extend the doc comment: what the effect slot is for, the "never throws, returns a + `Result`" contract, and why `Seed` is exempt. +4. Add the 5 spec cases from Acceptance below. +5. Run `npm run gen:behaviour-spec` — **new `it()` titles mean the drift check fails without + it** (see Risks). +6. Update this ticket's `Status:` to `done` and the README's RD-05 row to `done`. +7. Commit all of it together. + +## Acceptance criteria + +Five new plain-function spec cases, no TestBed: + +``` +- fires the effect when the store enters the tag +- does not fire when the tag is unchanged +- does not fire for a Seed message +- a dispatch from inside the effect lands +- the narrowed state is passed to the effect +``` + +Both existing cases still present and passing — in particular +`dispatch from inside an effect does not self-loop`, which is the regression guard for +decision 7. + +```bash +npm test # exits 0 +npm run ci # exits 0 +``` + +Type-level proof that decision 2 holds, i.e. the old call shape still compiles: + +```bash +npm run typecheck # exits 0 — store.spec.ts:8's createStore(0, ...) must still type-check +``` + +## Verification + +`npm run ci`. This ticket touches no story, no `.mdx`, and no component, so `--full` is not +required. + +## Out of scope + +- Migrating any call site. RD-06 (the 2 single-step forms, which is also a bug fix) and RD-08 + (the 3 wizards). +- Adding a `Primary` message to any machine. That is RD-07. +- The full Elm `reduce -> [state, Cmd]` refactor. Rejected in PLAN.md 1a, with reasons; + recorded there as the documented upgrade path if effects ever need asserting inside a domain + spec. +- `upload.machine.ts`'s `type:` discriminant. That is optional RD-35. + +## Risks + +- **The Storybook trap.** All 5 components mount their `Submitting`/`Indienen` state via + `Seed` in stories that use a real `provideHttpClient()` with **no request mocking** + (`besluit-form.stories.ts:33`, `herregistratie-wizard.stories.ts:70`, + `intake-wizard.stories.ts:38`, `change-request-form.stories.ts:34`, + `registratie-wizard.stories.ts:88`). The `Seed` exemption is what stops them firing real + calls and reddening `storybook-a11y`. Nothing migrates in this ticket, so the trap does not + fire yet — but the exemption must be implemented and documented **here**, because RD-06 is + where it would otherwise bite. +- **Two dispatch sites live inside Angular `effect()`s** — `intake-wizard.component.ts:363` + (`SetPolicy`) and `registratie-wizard.component.ts` (`PrefillAdres`). Both land on an + unchanged tag, so nothing fires, and both are already `untracked`. **Never key an effect on + an editing tag** — that is the livelock. +- **`behaviour-spec.mdx` drift.** `scripts/ci-local.sh` regenerates + `libs/shared/docs/behaviour-spec.mdx` from a path-sorted walk of spec titles and fails on + drift. New `it()` titles must be accompanied by `npm run gen:behaviour-spec` in the same + commit. +- **`snippets.generated.ts` drift.** Verified: `store.ts` carries **no** `// #region showcase:` + marker, so this ticket cannot cause snippet drift. (`remote-data.ts:30` has + `showcase:fold`, which RD-11 and RD-17 must respect — not this ticket.) diff --git a/docs/project/readable-codebase/README.md b/docs/project/readable-codebase/README.md index 0a24fcb..fb39439 100644 --- a/docs/project/readable-codebase/README.md +++ b/docs/project/readable-codebase/README.md @@ -99,7 +99,7 @@ two. Note that RD-15 exists because 22 abandoned agent worktrees are still on di | RD-02 | `max-lines` rule + `reportUnusedDisableDirectives` + 7 disables | 01 | | done | | RD-03 | `overzicht` context: page + 2 nav sections, boundary edge, admin-links token | 02 | yes | done | | RD-04 | Story titles to `Domein//`; add the missing stories | 03 | yes | todo | -| RD-05 | `createStore` gains the effect map + specs | 02 | | todo | +| RD-05 | `createStore` gains the effect map + specs | 02 | | done | | RD-06 | **Bug fix:** 2 single-step forms to the effect map + retry affordance | 05 | yes | todo | | RD-07 | Add `Primary` to the 3 wizard machines + specs | 05 | | todo | | RD-08 | Migrate the 3 wizards to the effect map + `Primary` | 07 | yes | todo | diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 725d977..8301797 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 505 frontend behaviours across +**is** the suite, reshaped for a business reader. 510 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -752,6 +752,11 @@ classes. - applies the pure update on dispatch - dispatch from inside an effect does not self-loop +- fires the effect when the store enters the tag +- does not fire when the tag is unchanged +- does not fire for a Seed message +- a dispatch from inside the effect lands +- the narrowed state is passed to the effect #### currentRole (dev mechanism) diff --git a/libs/shared/src/application/store.spec.ts b/libs/shared/src/application/store.spec.ts index e93958b..dc59be0 100644 --- a/libs/shared/src/application/store.spec.ts +++ b/libs/shared/src/application/store.spec.ts @@ -29,4 +29,75 @@ describe('createStore', () => { expect(runs).toBe(1); // effect ran once; its own dispatch did not retrigger it expect(store.model()).toBe(1); }); + + type ToggleModel = { tag: 'Off' } | { tag: 'On' }; + type ToggleMsg = { tag: 'Seed' } | { tag: 'Flip' } | { tag: 'Stay' }; + + function reduceToggle(model: ToggleModel, msg: ToggleMsg): ToggleModel { + switch (msg.tag) { + case 'Flip': + return model.tag === 'Off' ? { tag: 'On' } : { tag: 'Off' }; + case 'Seed': + return { tag: 'On' }; // mounts straight into 'On', e.g. restoring a draft + case 'Stay': + return model; + } + } + + it('fires the effect when the store enters the tag', () => { + let fired = false; + const store = createStore({ tag: 'Off' }, reduceToggle, { + On: () => (fired = true), + }); + + store.dispatch({ tag: 'Flip' }); + + expect(fired).toBe(true); + }); + + it('does not fire when the tag is unchanged', () => { + let fired = false; + const store = createStore({ tag: 'Off' }, reduceToggle, { + Off: () => (fired = true), + }); + + store.dispatch({ tag: 'Stay' }); // Off -> Off, no tag change + + expect(fired).toBe(false); + }); + + it('does not fire for a Seed message', () => { + let fired = false; + // Off -> On is a real tag change, but Seed is the mount/restore message + // (e.g. a Storybook story or a resumed draft) and must stay exempt. + const store = createStore({ tag: 'Off' }, reduceToggle, { + On: () => (fired = true), + }); + + store.dispatch({ tag: 'Seed' }); // Off -> On + + expect(store.model()).toEqual({ tag: 'On' }); + expect(fired).toBe(false); + }); + + it('a dispatch from inside the effect lands', () => { + const store = createStore({ tag: 'Off' }, reduceToggle, { + On: (_s, s) => s.dispatch({ tag: 'Flip' }), // On -> Off, from inside the effect + }); + + store.dispatch({ tag: 'Flip' }); // Off -> On, fires the effect above + + expect(store.model()).toEqual({ tag: 'Off' }); + }); + + it('the narrowed state is passed to the effect', () => { + let seen: ToggleModel | undefined; + const store = createStore({ tag: 'Off' }, reduceToggle, { + On: (state) => (seen = state), + }); + + store.dispatch({ tag: 'Flip' }); + + expect(seen).toEqual({ tag: 'On' }); + }); }); diff --git a/libs/shared/src/application/store.ts b/libs/shared/src/application/store.ts index 0e55de7..7ea33f0 100644 --- a/libs/shared/src/application/store.ts +++ b/libs/shared/src/application/store.ts @@ -17,17 +17,62 @@ export interface Store { dispatch(msg: Msg): void; } +/** + * The effect map: one optional handler per state tag, run when the store + * enters that tag (see the trigger rule on `createStore` below). Resolves to + * `never` for a tagless `Model` (e.g. `Model = number` in `store.spec.ts`), so + * a plain-value store still compiles without ever supplying effects. + * + * An effect body must never throw: end it in a `Result` from + * `runSubmit`/`runResult` (`submit.ts`) and let the failure travel as a + * dispatched message, not an exception. `dispatch` floats the effect's + * promise, exactly as the `runIfSubmitting()` call it replaces did. + */ +export type StoreEffects = Model extends { tag: string } + ? { + [K in Model['tag']]?: ( + state: Extract, + store: Store, + ) => unknown; + } + : never; + +function hasTag(value: unknown): value is { tag: unknown } { + return typeof value === 'object' && value !== null && 'tag' in value; +} + export function createStore( init: Model, update: (model: Model, msg: Msg) => Model, + effects?: StoreEffects, ): Store { const model = signal(init); - return { + const store: Store = { model: model.asReadonly(), - // Use `.update` (raw current value, no tracked read) not `set(update(model(), …))`: - // dispatch is a command and must never subscribe its caller to `model`. Reading - // `model()` here inside an effect that also dispatches makes the effect depend on - // its own write and livelock the main thread (crashed the upload wizards). - dispatch: (msg) => model.update((m) => update(m, msg)), + dispatch: (msg) => { + let prev!: Model; + let next!: Model; + // Use `.update` (raw current value, no tracked read) not `set(update(model(), …))`: + // dispatch is a command and must never subscribe its caller to `model`. Reading + // `model()` here inside an effect that also dispatches makes the effect depend on + // its own write and livelock the main thread (crashed the upload wizards). + model.update((m) => { + prev = m; + next = update(m, msg); + return next; + }); + + // Fire the entered tag's effect, but only when the store actually entered it + // (prev.tag !== next.tag) and the message is not `Seed` — the mount/restore + // message in every machine that has one. Without the `Seed` exemption, a + // component that mounts straight into `Submitting` (Storybook, a resumed + // draft) would fire the effect on load, not on user action. + if (!hasTag(next) || !hasTag(prev) || prev.tag === next.tag) return; + if (hasTag(msg) && msg.tag === 'Seed') return; + const handler = (effects as Record | undefined)?.[String(next.tag)] as + ((state: Model, store: Store) => unknown) | undefined; + handler?.(next, store); + }, }; + return store; }