Skip to content

refactor!: remove encode(), leaving toJSON() as the only projection - #3

Merged
btravers merged 1 commit into
mainfrom
refactor/encode-naming
Aug 6, 2026
Merged

refactor!: remove encode(), leaving toJSON() as the only projection#3
btravers merged 1 commit into
mainfrom
refactor/encode-naming

Conversation

@btravers

@btravers btravers commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Supersedes the original rename approach. Per review, the method is removed
rather than renamed — the naming collision it was meant to fix disappears when
the method does not exist.

Why removal beats renaming

encode() and toJSON() returned byte-identical data:

spread     : {"id":"…","cachedSummary":"leak me"}
encode()   : {"id":"…"}
toJSON()   : {"id":"…"}

Two public methods, one value — the alias CONTRIBUTING.md exists to prevent.

toJSON() is the one that has to stay. It is not a name this package chose; it
is the hook JSON.stringify looks for. Without it, serializing an entity walks
own enumerable properties and leaks a subclass's own instance fields — the same
bug spread has, as the first line above shows. So encode() was the removable
one.

It was also genuinely misnamed: it returned the stored (decoded) shape
while the exported Encoded<T> helper names the wire shape. For an entity
using decoded: { omit, add } these differ — Encoded<typeof ApiKey> carries
secret, the projection carries fingerprint and no secret. That mismatch is
why decode(x.encode()) never round-tripped, and the README now frames the
pairing positively (make(x.toJSON())) instead of apologising for a name.

Changes

  • entity.tsencode() deleted. toJSON, equals and update now route
    through a module-private project, so the internals do not depend on a
    serialization hook a subclass is free to override.
  • types.tsencode() removed from BaseInstance; the DeepReadonly
    comment updated to reference the one remaining projection.
  • Specs, entity.test-d.ts, both READMEs, CLAUDE.md — all call sites and
    prose updated (38 references).
  • entity.test-d.ts gains a @ts-expect-error pinning that encode() is gone.
    That assertion passing tsc is the proof the removal is complete.
  • CLAUDE.md corrected while here: it said "Five source modules" and referenced
    encode(); freeze.ts from fix: make entity data deeply immutable #7 makes six.

Also: entityName documented

Finding 5's second half. It was tested but documented nowhere in the root
README. Added, with the reason all three tag spellings coexist without being
aliases: _tag is instance-side and what P.tag(...) matches; entityName is
the only path for code holding the class and no instance; both derive from the
single Entity(tag) declaration.

Gate

format --check, lint, typecheck (both passes), test (76, 9 files),
knip, build — all green. Rebased onto current main, so the earlier
conflict with #7 is gone.

Copilot AI lite review requested due to automatic review settings August 6, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR resolves an API naming mismatch in the entity instance surface by renaming the instance method encode() (which returned the stored/decoded shape) to toStored(), aligning the name with the data shape it actually returns and the make() round-trip it supports.

Changes:

  • Renamed the instance method encode() to toStored() across the implementation and type surface.
  • Updated internal uses (toJSON(), equals(), update()) to delegate to toStored().
  • Updated documentation, specs, and added root README documentation for the existing entityName static.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
README.md Replaced encode() references with toStored(), reframed round-trip guidance, and documented entityName usage.
packages/entity/src/types.ts Updated the shared instance interface surface from encode() to toStored().
packages/entity/src/equality.spec.ts Updated equality test to use toStored() instead of encode().
packages/entity/src/entity.ts Renamed the instance method implementation to toStored() and updated internal call sites (toJSON, equals, update).
packages/entity/src/entity.spec.ts Updated instance-method behavior tests to reflect toStored().
packages/entity/src/decoded.spec.ts Updated decoded/omitted/add behavior tests and round-trip test wording to use toStored().
packages/entity/README.md Updated package README method docs from encode() to toStored() and clarified entityName as _tag’s static counterpart.
.changeset/silly-donuts-smoke.md Added a breaking-change changeset describing the rename and migration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/entity/src/types.ts Outdated
@btravers
btravers force-pushed the refactor/encode-naming branch from c8890ab to f907363 Compare August 6, 2026 19:43
@btravers btravers changed the title refactor!: rename encode() to toStored() refactor!: remove encode(), leaving toJSON() as the only projection Aug 6, 2026
@btravers
btravers requested a lite review from Copilot August 6, 2026 19:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@btravers
btravers merged commit 64b6ff2 into main Aug 6, 2026
1 check failed
@btravers
btravers deleted the refactor/encode-naming branch August 6, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants