refactor(shared): delete unwrapOk, the unadopted test value-object helper (RB-33)
unwrapOk had zero consumers in apps/ or libs/ since ADR-0006 shipped it. The one call site the finding named already satisfies the ADR's real rule (call the real parser, never a cast) with an inline guard, so adding a manufactured first caller was not the better fix. This commit deletes the helper and its file, and updates the one doc sentence that named it. The finding's call site is unchanged. See rb-33.md for the full adopt-or-delete reasoning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,100 @@
|
||||
# RB-33 — `unwrapOk`: adopt or delete
|
||||
|
||||
Status: **implemented** · 2026-08-28 · Source finding: `06-adr-conformance.md` ADR-C-011 ·
|
||||
`99-backlog.md` RB-33
|
||||
|
||||
## Decision: delete
|
||||
|
||||
The ticket names this "adopt or delete", not "adopt", and asks for the judgment call, not
|
||||
the default. I deleted `unwrapOk`.
|
||||
|
||||
## Why delete, not adopt
|
||||
|
||||
`unwrapOk` (`libs/shared/src/testing/value-object.ts`) has had zero consumers across the
|
||||
whole codebase since ADR-0006 shipped it, except its own definition and one sentence in
|
||||
`libs/shared/docs/testing.mdx`. I verified this before changing anything:
|
||||
|
||||
```
|
||||
grep -rn "unwrapOk" apps libs --include=*.ts --include=*.mdx
|
||||
libs/shared/docs/testing.mdx:92: ...unwrapOk(parseX(raw))...
|
||||
libs/shared/src/testing/value-object.ts:9:export function unwrapOk<E, T>(...)
|
||||
libs/shared/src/testing/value-object.ts:11: throw new Error(`unwrapOk: ...`);
|
||||
```
|
||||
|
||||
The one call site the finding names,
|
||||
`apps/ssp/src/app/registratie/application/submit-change-request.spec.ts`, still has the
|
||||
exact hand-rolled guard the finding quotes:
|
||||
|
||||
```ts
|
||||
const telefoon = parseTelefoonnummer('0612345678');
|
||||
if (!telefoon.ok) throw new Error('fixture phone should parse');
|
||||
```
|
||||
|
||||
I also checked whether any other spec has the same shape, in case the finding's "one call
|
||||
site" undercounted the real duplication:
|
||||
|
||||
```
|
||||
grep -rln "if (!.*\.ok)\s*throw" apps libs --include=*.spec.ts
|
||||
apps/ssp/src/app/registratie/application/submit-change-request.spec.ts
|
||||
```
|
||||
|
||||
Only this one file, anywhere. There is no cast (`'x' as Telefoonnummer`) to close off
|
||||
either — the spec already calls the real `parseTelefoonnummer` and checks `.ok` before
|
||||
touching `.value`. ADR-0006 §3's actual requirement ("never a cast") is already met by the
|
||||
inline code, with or without the helper.
|
||||
|
||||
Weighing it honestly:
|
||||
|
||||
- **For adopt:** it is a one-line change, and the ADR's own worked example literally shows
|
||||
this exact call. Doing it would make the finding's "zero adopters" claim technically
|
||||
false.
|
||||
- **For delete:** a helper that gains its _only_ real-codebase consumer by an agent adding
|
||||
that one call site as an act of ticket compliance is not organic adoption — it is
|
||||
manufacturing a usage to justify keeping the file. `unwrapOk` has sat available, exported,
|
||||
and documented since ADR-0006 (well before this session) without a single spec reaching
|
||||
for it on its own. One caller, forever, is not "removing duplication" (the stated point
|
||||
of a shared test helper) — there is no duplication with only one occurrence. The inline
|
||||
guard is also arguably clearer here: its error message (`'fixture phone should parse'`)
|
||||
names the actual fixture, where `unwrapOk`'s generic message
|
||||
(`unwrapOk: expected ok, got error: ...`) does not.
|
||||
|
||||
Delete wins: it removes dead, unadopted code and its stale doc reference, changes no
|
||||
runtime behaviour anywhere, and costs nothing to reverse if a second real need for this
|
||||
idiom shows up later (three lines, trivial to re-add against actual duplication instead of
|
||||
a single hypothetical site).
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `libs/shared/src/testing/value-object.ts` | Deleted. Its only export, `unwrapOk`, is what this ticket removes; the file had nothing else in it. |
|
||||
| `libs/shared/docs/testing.mdx` | Rewrote the sentence that named `unwrapOk` and the deleted file's path. It now states the same rule in plain terms — call the real `parse*` and check `.ok`, never a cast — and keeps the `RemoteData` half of the sentence pointing at `remote-data.ts` (unchanged, still in use). |
|
||||
| `apps/ssp/src/app/registratie/application/submit-change-request.spec.ts` | **Not touched.** Its inline guard already satisfies ADR-0006 §3; this is the "delete" branch, so the fixture-construction behaviour stays exactly as it was. |
|
||||
| `99-backlog.md` | RB-33's status cell: `open` → `implemented`. |
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
`docs/reference/architecture/0006-test-data-builders.md` (the ADR itself) still shows
|
||||
`unwrapOk` in its worked example and decision table. That is deliberate: RB-33 is a code
|
||||
ticket, not one of the five ADR-fix tickets that need architect sign-off
|
||||
(`06-adr-conformance.md`'s "ADR-fix tickets" section). The ADR's illustrated pattern
|
||||
("call the real parser, unwrap through a checked path, never a cast") is still the correct
|
||||
principle — this ticket only removes one now-unused concrete implementation of it, which
|
||||
the inline guard in `submit-change-request.spec.ts` already satisfies without the named
|
||||
helper. Amending the ADR's own text is out of this ticket's scope and is left for a future
|
||||
ADR-fix ticket if one is ever raised. The finding document (`06-adr-conformance.md`) and the
|
||||
historical WP-70/WP-71 backlog notes that mention `unwrapOk` are left as-is — they are
|
||||
records of what was true when written, not living code.
|
||||
|
||||
No other file in `libs/shared/src/testing/` was touched (`expect-tag.ts`, `machine.ts`,
|
||||
`remote-data.ts` are all unrelated and still have real consumers).
|
||||
|
||||
## Verification
|
||||
|
||||
- `grep -rn "unwrapOk" apps libs --include=*.ts --include=*.mdx` — zero occurrences.
|
||||
- `apps/ssp/src/app/registratie/application/submit-change-request.spec.ts` — unchanged file,
|
||||
still passes (see `npm run ci` result below).
|
||||
- No new test added. The ticket is a deletion of unused code plus a doc-sentence rewrite;
|
||||
the surviving inline guard in the spec is exercised the same way it always was, by the
|
||||
spec's three existing `it` blocks.
|
||||
- `npm run ci` (foreground): see the session report for the exit code and step count.
|
||||
Reference in New Issue
Block a user