Skip to content

fix: bind instance and ~standard to the class they are read from - #2

Closed
btravers wants to merge 2 commits into
mainfrom
fix/instance-subclass-identity
Closed

fix: bind instance and ~standard to the class they are read from#2
btravers wants to merge 2 commits into
mainfrom
fix/instance-subclass-identity

Conversation

@btravers

@btravers btravers commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

The bug

instance and ~standard were lazy accessors that overwrote themselves
with a plain data property on the receiver the first time they ran. That made
identity stable, but it is first-read-wins per receiver, and a data property
defined on a class is inherited by any plain subclass that has no
Entity(...) call of its own:

class Org extends Entity("Org")({ id: OrgId, slug: Slug }) {}
Org.instance.parse(raw);        // reads the PARENT first → the getter on Org
                                //   is replaced by Org's own data property
class Sub extends Org {}        // a bare JS subclass, no Entity(...) of its own
Sub.instance.parse(raw);        // → builds an Org, NOT a Sub
                                //   `built instanceof Sub` === false

No error, no warning — silently the wrong class. The flaw was documented in a
comment in instance.ts; it reproduced exactly as written.

The fix

Keep the accessor in place permanently and memoise the built schema per
receiver
in a module-level WeakMap keyed by the class object. Every read
re-enters the getter with this bound to the class actually read and gets
that class's own entry, so:

  • X.instance === X.instance — built once, stable identity (the existing
    "built once and reused" test still passes unchanged)
  • Sub.instance !== Org.instance, and Sub.instance.parse(raw) instanceof Sub
    is true regardless of read order
  • keys are constructors, so a discarded class stays collectable

~standard no longer caches separately: it reads through this.instance, so
the two can never disagree about which class they decode to, and zod hangs
~standard off the schema at construction, which makes it stable for free.
Both properties stay non-enumerable (absent from Object.keys and spread) and
configurable.

Why an accessor at all is unchanged and still documented: the consuming class
does not exist when the entity builder runs, so a plain value would close over
the base constructor and X.instance.parse(...) would build a base instance,
failing instanceof X. The getter reads this from the access site.

Also

  • The long comment in instance.ts now describes the WeakMap mechanism and
    records why the self-overwrite was abandoned.
  • Tests in instance.spec.ts: both read orders, distinct-and-stable
    instance/~standard per class, a two-level chain, and a check that nothing
    is stamped onto the class as an own or enumerable property. Three of them
    fail against the old implementation.
  • README states the subclass behaviour under the statics table, including the
    standing type-level caveat (a static property cannot repolymorphize per
    subclass — the comment on EntityStatic["instance"] in types.ts is
    unaffected by this fix).

Gate

pnpm format, lint, typecheck (incl. .test-d.ts), test (66 passed),
knip, build — all pass.

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

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.

The lazy accessors overwrote themselves with a data property on the
receiver, which a plain `class Y extends X {}` then inherited: reading
`X.instance` first left `Y.instance` resolving to `X`'s schema, so
`Y.instance.parse(...)` silently built an `X`. Memoise per receiver in a
module-level WeakMap and keep the accessors in place instead, so identity
stays stable per class and read order no longer decides the outcome.
@btravers
btravers force-pushed the fix/instance-subclass-identity branch from c24af18 to 4c0189c Compare August 6, 2026 20:45
@btravers

btravers commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Superseded. Entity subclassing is being prohibited rather than made correct (see the follow-up PR), so the case this fixed — Sub.instance silently building a Parent — can no longer arise.

With second-level subclassing blocked at construction, the original self-overwriting getter on main is correct: it was only ever wrong when a bare class Sub extends Entity {} inherited a stamped property, and that class can no longer be constructed. The WeakMap memoisation this PR added would be dead weight.

The subclass-identity tests here go too, for the same reason. The genuinely useful finding from this branch — that Object.getOwnPropertyDescriptor(SomeEntity, "instance") is undefined because the accessor lives on the Entity(...) base — is preserved in the follow-up.

@btravers btravers closed this Aug 6, 2026
@btravers
btravers deleted the fix/instance-subclass-identity 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