Uh oh!
There was an error while loading. Please reload this page.
refactor: cut the hand-rolled deepEqual and dead scaffolding - #67
Merged
Conversation
`equals` delegates to `node:util`'s `isDeepStrictEqual` instead of a 172-line traversal. Every case `equal.spec.ts` pins passes against it unchanged — bigint, Set/Map by contents, nested key order, typed arrays and ArrayBuffer bytewise, RegExp, Date, and both cyclic-value tests — with one difference: `+0`/`-0` now compare unequal (Object.is) where SameValueZero made them equal. No test pinned that. The import makes the package Node-only. Also removed, all verified against the full gate: - two of the three JSON-LD blocks in the docs head; `WebSite` and `Organization` restated title/url/logo VitePress already emits - the `test:types` script/turbo task/script triple — CI runs format, lint, typecheck, test, knip, build, and `typecheck` already runs the `.test-d.ts` pass - `IsAtLeastAsWideAs`, inlined at its two uses in `IsNarrowLiteral` - `RootInstance`, now `ConstructedInstance<string, S, A>` rather than a second spelling of the same intersection — measured 47 bytes SMALLER in the emitted `index.d.mts`, with zero `GeneratedKeys<`/`ImmutableKeys<` in the consumer emit set - `schemaOf`, inlined at its one call site in `shape()` - the `Array.isArray(values)` branch in `discriminantValues`: zod v4 gives a Set for `z.literal(...).values` and `undefined` for `z.enum(...).values`, so no schema reached it A shared vitest config was tried and reverted: knip discovers spec files through each package's own config, so consolidating reported all 16 spec files and 5 devDependencies as unused.
There was a problem hiding this comment.
Pull request overview
This PR refactors @btravstack/entity to remove dead scaffolding and replace the hand-rolled deep equality implementation with Node’s node:utilisDeepStrictEqual, while also pruning unused scripts/tasks and a few type/util helpers.
Changes:
- Replace the previous
deepEqualtraversal withnode:util’sisDeepStrictEqualforEntity#equals, and update tests accordingly (making the package Node-only). - Remove the
test:typesturbo task and package scripts, relying on the existingtypecheckpipeline to run the.test-d.tspass. - Inline/remove small helpers and simplify a few internal types/utilities (
schemaOf,IsAtLeastAsWideAs,RootInstancealiasing).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Removes the test:types task from the turbo pipeline. |
| packages/entity/src/union.ts | Removes an unreachable discriminant .values array fallback (zod v4 measured behavior). |
| packages/entity/src/types.ts | Simplifies RootInstance to reuse ConstructedInstance<string, S, A>. |
| packages/entity/src/shape.ts | Inlines schemaOf behavior using isFieldSpec for unwrapping field specs. |
| packages/entity/src/field.ts | Deletes schemaOf helper (now inlined at call site). |
| packages/entity/src/equal.ts | Deletes the hand-rolled deep equality implementation. |
| packages/entity/src/equal.spec.ts | Switches equality spec assertions to use node:utilisDeepStrictEqual. |
| packages/entity/src/entity.ts | Implements equals via isDeepStrictEqual and updates the rationale doc comment. |
| packages/entity/package.json | Removes test:types script (keeps .test-d.ts under typecheck). |
| package.json | Removes root-level test:types script. |
| docs/.vitepress/config.ts | Removes redundant JSON-LD blocks from the docs head. |
| CLAUDE.md | Updates architecture notes to reflect the removal of equal.ts and Node-only equality. |
| .changeset/lazy-pans-brake.md | Documents the Node-only change and the +0 vs -0 equality behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
A ponytail audit of the whole tree, applied. +25 / −231 lines, no new dependencies.
The one decision to make
equalsnow delegates tonode:util'sisDeepStrictEqualinstead of a 172-line hand-rolled traversal. Every caseequal.spec.tspins passes against it unchanged — bigint,Set/Mapby contents, nested key order, typed arrays andArrayBufferbytewise,RegExp,Date, and both cyclic-value tests (including the Set-matching poison cases).Two consequences, both in the changeset:
node:utilshim will no longer resolve it. There is nobrowserfield today and nothing else imports anode:builtin, so this is the first such constraint. If browser consumers matter, this commit is the one to drop.+0and-0now compare unequal (Object.is) where SameValueZero made them equal. No test pinned it.The rest
WebSiteandOrganizationrestated title/url/logo VitePress already emits; "entity" is not an Organizationtest:typesscript / turbo task / package scripttypecheckalready runs the.test-d.tspass. Confirmed againstbtravstack/config'sci-reusable.ymlIsAtLeastAsWideAsIsNarrowLiteralRootInstance's duplicate intersectionConstructedInstance<string, S, A>— same type, one definitionschemaOfshape()Array.isArray(values)indiscriminantValuesSetforz.literal(...).valuesandundefinedforz.enum(...).values— measured, no schema reached itTried and reverted
A shared root
vitest.shared.tsreplacing four byte-identical configs. Tests ran fine, but knip discovers spec files through each package's own vitest config, so consolidating made it report all 16 spec files and 5 devDependencies as unused. Teaching knip costs more config than the 21 duplicate lines it saves.Verification
Full gate green in gate order:
format --check,lint,typecheck(all fourbilling-domainsteps, both compilers, plus the emitted-.d.tscheck), 179 + 18 tests,knip,build.The declaration-emit acceptance rules were re-measured rather than assumed: zero
GeneratedKeys</ImmutableKeys<in the consumer emit set, and theRootInstancealias madeindex.d.mts47 bytes smaller (40,768 → 40,721), measured by building both ways.Not cut, checked and justified in the audit:
freeze.ts's schema-directed passthrough walk (every branch is spec-pinned, and removing it re-freezes objects the caller still owns), the*Srcalias block inentity.ts, the ten top-level type re-exports inindex.ts, and the double-compiler emit gate.