feat: add an optional effect map to createStore (RD-05)
createStore now takes a third, optional StoreEffects map. Each key is a Model tag. The store runs that tag's handler after update() returns, and only when the store enters the tag: the previous tag differs from the new tag, and the message is not Seed (the mount/restore message in every machine that has one). This closes the gap where a component had to call dispatch(msg) and then a private runIfSubmitting() by hand, or state got silently stuck. No call site changes here. RD-06 and RD-08 migrate the 5 components that duplicate that pattern today. The effect map is a conditional type, not a generic constraint, so a tagless Model (store.spec.ts's plain number store) still resolves it to never and needs no third argument. Both tag checks use a typeof/in guard for the same reason. Regenerated libs/shared/docs/behaviour-spec.mdx for the 5 new spec titles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Claude Sonnet 5
@@ -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, Msg> = Model extends { tag: string }
|
||||
? {
|
||||
[K in Model['tag']]?: (
|
||||
state: Extract<Model, { tag: K }>,
|
||||
store: Store<Model, Msg>,
|
||||
) => unknown;
|
||||
}
|
||||
: never;
|
||||
|
||||
export function createStore<Model, Msg>(
|
||||
init: Model,
|
||||
update: (model: Model, msg: Msg) => Model,
|
||||
effects?: StoreEffects<Model, Msg>,
|
||||
): Store<Model, Msg>;
|
||||
```
|
||||
|
||||
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<Model, Msg>` 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.)
|
||||
@@ -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/<Context>/<Name>`; 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 |
|
||||
|
||||
Reference in New Issue
Block a user