fix: correctness and DX hardening from a multi-perspective review - #41
Merged
Conversation
…aries Two measured correctness holes: - `deepEqual`'s cycle guard recorded every pair it entered and never forgot one that finished false, so `unorderedEqual`'s failed candidate matches poisoned later genuine comparisons — two Set fields with plainly different contents compared equal once their elements shared a subtree. The guard is now a stack of in-progress pairs, not a memo. - `deepFreeze`'s walk lost schema context at union, pipe and intersection boundaries, so a z.custom value nested inside one was frozen in place — mutating an object the caller still owns, the worse of the two errors by the module's own docstring. The walk now carries an "any of these branches" context that resolves ambiguity toward skipping. Also pins the documented-but-untested typed-array, ArrayBuffer, RegExp and tuple branches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…scriminants - `InvalidEntity.message` is rendered eagerly — entity name plus each issue's path and message — so a log line or failed assertion no longer prints a blank Error. New `Entity.renderIssue` and `Entity.keysOf` expose the same helpers for adapters building field-level responses. - `toJSON()` returns `DeepReadonly<Output>`: the projection is shallow, so nested containers are the instance's own frozen references, and the mutable type let `toJSON().tags.push(…)` compile and throw. - A duplicate union discriminant value across members is a declaration-time defect naming both members, instead of silently last-winning in `make` while zod threw lazily at the first parse. - The construction seal's property is `__useMakeOrFactoryInstead`, so the compile error on `new SomeEntity(…)` carries the fix. - Stale `consumer/` pointers updated to the emit-guards fixture; a byte-identical duplicated test in schema.spec.ts removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The top-level type-export list is six, not three; `Entity.Static` is documented; `updateInput` is output minus immutable and computed; the field rules license `.optional()` and one array level; the zod `^4.3.0` floor reaches the site; `toJSON()` reads as DeepReadonly. - The http-contract guide maps issues with `Entity.keysOf` and shows `Entity.renderIssue`; errors.md covers the populated message, the union's invalid-discriminant issue and the duplicate-discriminant declaration defect. - Two new pages: explanation/branded-fields — the argument for the nominal rule and the two blessed brand-minting patterns — and how-to/evolve-an-entity — add, rename and retire fields against stored rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens @btravstack/entity based on a full-project review, focusing on correctness fixes in core runtime helpers (deepEqual, deepFreeze, union dispatch), improving developer experience (clearer seal error, InvalidEntity.message, exported issue helpers), and syncing/expanding documentation to match the refined public surface and behaviors.
Changes:
- Fix correctness edge cases in
deepEqual,deepFreeze, andEntity.union(...)(including declaration-time duplicate-discriminant defects). - Improve DX and type honesty (readonly
toJSON(), populatedInvalidEntity.message, exportEntity.keysOf/Entity.renderIssue, seal property rename). - Update and extend docs (new “Branded fields” + “Evolve an entity” pages, reference/tutorial/how-to refresh) and add a changeset.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/entity/src/union.ts | Detect duplicate discriminant values at declaration time and throw a defect with both member names. |
| packages/entity/src/union.spec.ts | Add spec pinning duplicate-discriminant as a declaration-time defect. |
| packages/entity/src/types.ts | Rename seal property, make toJSON() return DeepReadonly<Output>, and update related type docs. |
| packages/entity/src/schema.spec.ts | Remove a duplicated/byte-identical test. |
| packages/entity/src/freeze.ts | Propagate schema context through pipe/union/intersection to avoid freezing passthrough values. |
| packages/entity/src/freeze.spec.ts | Add regression tests covering union/pipe/intersection/tuple passthrough behavior. |
| packages/entity/src/errors.ts | Populate InvalidEntity.message using rendered issues for better logs/assertions. |
| packages/entity/src/equal.ts | Fix cycle-guard handling to avoid false-positive equality in unordered structures. |
| packages/entity/src/equal.spec.ts | Add regression tests for prior poisoning bug and new typed-array/ArrayBuffer/RegExp comparisons. |
| packages/entity/src/entity.ts | Type toJSON() as DeepReadonly, export Entity.keysOf/Entity.renderIssue, and adjust related comments. |
| packages/entity/src/entity.test-d.ts | Update type-level tests to enforce readonly toJSON() behavior. |
| packages/entity/src/crud.spec.ts | Add specs for InvalidEntity.message and the new issue helper exports. |
| packages/entity/README.md | Sync schema member table to reflect updateInput excluding computed fields. |
| docs/typedoc.json | Adjust TypeDoc “intentionallyNotExported” list to match updated type surface. |
| docs/tutorial/getting-started.md | Document zod floor, clarify schema identity, link to new evolution guide. |
| docs/reference/types.md | Update reference to six declaration-emit type exports and their rationale. |
| docs/reference/schemas.md | Document updateInput excluding immutable + computed fields. |
| docs/reference/errors.md | Document InvalidEntity.message plus Entity.keysOf/Entity.renderIssue and union failure channels. |
| docs/reference/entry-points.md | Document toJSON(): DeepReadonly<Output> and mutable-copy guidance. |
| docs/reference/declaration.md | Clarify nominal-field rules and union error/defect behavior. |
| docs/how-to/test-domain-logic.md | Update snippet imports (remove unused match). |
| docs/how-to/persist-and-rehydrate.md | Add guidance about DeepReadonly projections and link to evolution guide. |
| docs/how-to/model-an-aggregate.md | Update examples and document union invalid-discriminant vs duplicate-defect behavior. |
| docs/how-to/http-contract.md | Use Entity.keysOf and document Entity.renderIssue for adapters. |
| docs/how-to/evolve-an-entity.md | New how-to page describing safe entity evolution against stored rows. |
| docs/explanation/sealed-construction.md | Document new seal property name as an instructional compiler error. |
| docs/explanation/peer-dependencies.md | Document measured zod floor and rationale for range. |
| docs/explanation/branded-fields.md | New explanation page for nominal-field constraints and brand-minting patterns. |
| docs/api/index.md | Sync API index prose to updated top-level type exports and new helpers. |
| docs/.vitepress/config.ts | Add new pages to sidebar navigation. |
| CLAUDE.md | Update internal guidance references from removed consumer/ to the emit-guards fixture. |
| .changeset/heavy-buses-repair.md | Add release notes for the behavior/type/DX changes as a minor bump. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A completed `true` may have relied on an enclosing pair that was still only provisionally assumed equal; when that assumption then fails, the remembered `true` wrongly matches the nested pair in a later genuine trial. Both directions are now pinned, and the guard removes each pair on exit regardless of outcome — pure stack semantics, matching the `Seen` docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes every confirmed defect from a full-project review (code quality, DX, documentation), plus the DX improvements and doc corrections that came out of it. All behaviour changes landed test-first; the reproductions are pinned in the specs.
Correctness
deepEqualfalse-positive equality. The cycle guard memoised failed comparisons, so twoSet/Mapfields with different contents compared equal once their elements shared a subtree (reproduced, now pinned). The guard is a stack of in-progress pairs, not a memo.deepFreezefroze caller-owned values under union branches.childSchemahad nounion/pipe/intersectioncases, so the walk lost context and frozez.customvalues nested below those boundaries — the exact harm the module exists to prevent. Context now flows through all three, resolving ambiguity toward skipping.toJSON()typed honestly. The projection is shallow — nested containers are the instance's frozen references — so the return is nowDeepReadonly<Output>instead of a mutable type that lettoJSON().tags.push(…)compile and throw.makeand a lazy zod throw at first parse.DX
InvalidEntity.messageis populated ("Organization: id: Invalid UUID; …") — no more blankErrorin logs and test failures.Entity.renderIssue/Entity.keysOfare exported for adapters.__useMakeOrFactoryInstead, sonew SomeEntity(…)'s compile error tells the reader what to do.Docs
Reference pages synced with the actual surface (six top-level type exports,
Entity.Static,updateInputminus computed, optional/array field rules, zod^4.3.0floor), how-to fixes (noUncheckedIndexedAccess-safe snippets, issue mapping via the new helpers), and two new pages: explanation/branded-fields and how-to/evolve-an-entity. Docs build is at zero warnings; every changed snippet was verified against current source.Housekeeping: stale
consumer/pointers updated to the emit-guards fixture (source + CLAUDE.md); a byte-identical duplicated test removed fromschema.spec.ts.Breaking (0.x minor, covered by the changeset)
The
toJSON()return type and the seal-property rename change emitted declarations; the consumer fixture compiles clean on both TypeScript 7.0.2 and 5.9.3.Test plan
format --check·lint·typecheck(all six targets incl. the 5.9.3 consumer pass) ·test(156 package + 22 example tests) ·knip·build(package + docs) — all green locally, in CI ordertoJSON(type-level)Related enhancement issues filed separately: #37, #38, #39, #40.
🤖 Generated with Claude Code