Eight sites hand-rolled `rd.tag === 'Success' ? rd.value : fallback`. Six take the new `successOr(rd, fallback)`, one takes the existing `successOf`, and one (`big-profile.store.ts`) uses the existing `map`, since it returns a RemoteData rather than an unwrapped value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
161 lines
7.9 KiB
Markdown
161 lines
7.9 KiB
Markdown
# RD-17 — Add `successOr`, and sweep the remaining inline unwraps
|
|
|
|
Status: done
|
|
Source: PLAN.md 2.3
|
|
|
|
## Why
|
|
|
|
`successOf` landed with the dashboard refactor and removed the repeated
|
|
`rd.tag === 'Success' ? rd.value : undefined` from five sites. **Eight more inline unwraps
|
|
remain**, and they do not all want the same helper — the fallbacks genuinely differ. One
|
|
helper is missing, and one site hand-rolls a function that already exists.
|
|
|
|
## Already in the working tree (an interrupted session did steps 1-2)
|
|
|
|
`git status` shows four modified files. Do not redo this work; check it, then continue.
|
|
|
|
- `remote-data.ts` — `successOr` exists at 119 with the three-parameter signature and a doc
|
|
comment. Done.
|
|
- `remote-data.spec.ts` — `successOr` cases mirror `successOf`'s. Done.
|
|
- `behandeling/ui/beoordeling.page.ts` — converted to `successOf`. Done.
|
|
- `behandeling/ui/werkvoorraad.page.ts` — converted to `successOr(rd, [])`. Done.
|
|
|
|
Six sites in the decision-2 table remain.
|
|
|
|
## Read first
|
|
|
|
- `libs/shared/src/application/remote-data.ts` — `successOf` at 109, `map` at 78, and the
|
|
`// #region showcase:fold` marker at 50-69. `successOr` goes next to `successOf`, well clear
|
|
of that region.
|
|
- `libs/shared/src/application/remote-data.spec.ts` — `successOf` already has cases; mirror
|
|
them.
|
|
|
|
## Decisions (pre-made, don't relitigate)
|
|
|
|
1. **Add `successOr` with three type parameters, not two:**
|
|
|
|
```ts
|
|
export function successOr<E, T, F>(rd: RemoteData<E, T>, fallback: F): T | F {
|
|
return rd.tag === 'Success' ? rd.value : fallback;
|
|
}
|
|
```
|
|
|
|
The third parameter is load-bearing. Call sites pass `[]` and `null`, neither of which is
|
|
assignable to `T`, so a two-parameter `successOr<E, T>(rd, fallback: T): T` would not
|
|
compile at those sites.
|
|
|
|
2. **The eight in-scope sites, and which helper each takes:**
|
|
|
|
| Site | Today | Becomes |
|
|
| ------------------------------------------------------- | -------------------------------------------------- | ------------------------------------- |
|
|
| `behandeling/ui/beoordeling.page.ts:78` | `? rd.value : undefined` | `successOf(rd)` |
|
|
| `behandeling/ui/werkvoorraad.page.ts:61` | `? rd.value : []` | `successOr(rd, [])` |
|
|
| `registratie/ui/admin-cases.page.ts:83` | `? rd.value : []` | `successOr(rd, [])` |
|
|
| `beheer/src/ui/audit.page.ts:99` | `? rd.value : []` | `successOr(rd, [])` |
|
|
| `shared/application/feature-flags.store.ts:27` | `? rd.value : []` | `successOr(rd, [])` |
|
|
| `registratie/ui/dashboard/mijn-aanvragen.section.ts:93` | `? sortForDashboard(rd.value) : []` | `sortForDashboard(successOr(rd, []))` |
|
|
| `registratie/ui/registratie-wizard/…component.ts:521` | `? rd.value : null` | `successOr(rd, null)` |
|
|
| `registratie/application/big-profile.store.ts:57` | `? { tag: 'Success', value: rd.value ?? [] } : rd` | **`map(rd, (v) => v ?? [])`** |
|
|
|
|
3. **The last row is the interesting one.** `big-profile.store.ts:57` re-wraps a `Success`
|
|
and passes everything else through — that is exactly `map`, which has existed in this file
|
|
since before the arc started. Use the existing function; do not reach for `successOr`
|
|
there, because the site returns a `RemoteData`, not an unwrapped value.
|
|
|
|
4. **`mijn-aanvragen.section.ts` folds a map into its unwrap.** `sortForDashboard(successOr(rd, []))`
|
|
is equivalent because `sortForDashboard([])` is `[]`, and it reads better than nesting
|
|
`map`. Keep the sort outside.
|
|
|
|
5. **Leave the two boolean predicates alone.** `access.store.ts:36`
|
|
(`&& rd.value.includes(capability)`) and `feature-flags.store.ts:51`
|
|
(`&& (rd.value.find(…)?.enabled ?? false)`) answer a yes/no question rather than unwrapping
|
|
a value. `successOr(rd, []).includes(x)` would work but allocates an array to answer a
|
|
boolean, and reads no better. Not a win.
|
|
|
|
6. **Four sites inside `remote-data.ts` itself are not call sites** — they are the bodies of
|
|
`map`, `andThen`, `successOf` and the new `successOr`. Obviously do not rewrite a function
|
|
in terms of itself.
|
|
|
|
7. **Leave the six spec-file occurrences alone.** `aanvragen.store.spec.ts` and
|
|
`admin-cases.store.spec.ts` use `s.tag === 'Success' && s.value.map(…)` inside `expect(…)`.
|
|
That is an assertion idiom; replacing it would obscure what the test checks.
|
|
|
|
## Files
|
|
|
|
- `libs/shared/src/application/remote-data.ts` (+ `.spec.ts`) — the new helper
|
|
- The eight files in decision 2
|
|
|
|
## Steps
|
|
|
|
1. Add `successOr` next to `successOf` per decision 1, with a doc comment saying when to
|
|
reach for it rather than `successOf` or `map`.
|
|
2. Add spec cases mirroring `successOf`'s.
|
|
3. Convert the eight sites per the table. One file at a time; let the type-checker confirm
|
|
each.
|
|
4. Run `npm run gen:behaviour-spec` — new spec titles otherwise fail the drift check.
|
|
5. Update this ticket's `Status:` to `done` and the README's RD-17 row to `done`.
|
|
6. Commit all of it together.
|
|
|
|
## Acceptance criteria
|
|
|
|
Measured baselines, dry-run before handover. The ternary form appears **11** times in the
|
|
working tree; **4** of those are the bodies of `map`, `andThen`, `successOf` and `successOr`
|
|
inside `remote-data.ts` and must survive, so the target is exactly 4.
|
|
|
|
```bash
|
|
git grep -c "tag === 'Success' ?" -- apps libs | awk -F: '{s+=$NF} END {print s}' # is 11 -> MUST be 4
|
|
git grep -c "tag === 'Success' ?" -- libs/shared/src/application/remote-data.ts # MUST still be 4
|
|
```
|
|
|
|
The new helper exists and is used:
|
|
|
|
```bash
|
|
git grep -c "export function successOr" -- libs/shared/src/application/remote-data.ts # MUST be 1
|
|
git grep -l "successOr" -- apps libs | wc -l # >= 7
|
|
```
|
|
|
|
The two predicates and the specs are untouched (decisions 5 and 7):
|
|
|
|
```bash
|
|
git grep -c "tag === 'Success' &&" -- apps libs | awk -F: '{s+=$NF} END {print s}' # unchanged: 8
|
|
```
|
|
|
|
`big-profile.store.ts` uses the existing `map`, not a new helper (decision 3):
|
|
|
|
```bash
|
|
git grep -n "map(" -- apps/ssp/src/app/registratie/application/big-profile.store.ts # >= 3 (2 existing + the new one)
|
|
```
|
|
|
|
```bash
|
|
npm run ci # exits 0
|
|
```
|
|
|
|
## Verification
|
|
|
|
`npm run ci`. This edits no story and no `.mdx`, but it **does** edit
|
|
`libs/shared/src/application/remote-data.ts`, which is not under `libs/shared/src/ui/**` — so
|
|
`--full` is not required by the README's rule.
|
|
|
|
Verified for you: the `// #region showcase:fold` marker sits at lines 50-69, well above
|
|
`successOf` at 109, so adding a function there cannot cause snippet drift. If you move
|
|
anything inside that region, run `npm run gen:snippets` in the same commit.
|
|
|
|
## Out of scope
|
|
|
|
- The two boolean predicates (decision 5).
|
|
- The six spec-file assertions (decision 7).
|
|
- `remote-data.ts`'s own three internal uses (decision 6).
|
|
- Adding any further combinator. `successOf`, `successOr`, `map`, `map2` and `andThen` cover
|
|
every site here; a sixth would be speculative.
|
|
|
|
## Risks
|
|
|
|
- **Do not give `successOr` two type parameters.** Call sites pass `[]` and `null`; a
|
|
`fallback: T` signature fails to compile at exactly the sites this ticket exists to fix.
|
|
- **`big-profile.store.ts:57` is a `map`, not a `successOr`.** It returns a `RemoteData`. Using
|
|
`successOr` there would change the member's type and break its consumers.
|
|
- **`behaviour-spec.mdx` drift** from the new spec titles. Run `gen:behaviour-spec` in the
|
|
same commit.
|
|
- **Watch the `?? []` inside `big-profile.store.ts:57`.** The value being mapped is
|
|
nullable; the `?? []` must move inside the `map` callback, not disappear.
|