Skip to content

chore: cover the published Node floor in CI and record two invisible couplings - #6

Merged
btravers merged 2 commits into
mainfrom
chore/review-cleanups
Aug 6, 2026
Merged

chore: cover the published Node floor in CI and record two invisible couplings#6
btravers merged 2 commits into
mainfrom
chore/review-cleanups

Conversation

@btravers

@btravers btravers commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Four small, independent cleanups from a code review. No runtime behaviour
changes.

1. pnpm format --check no longer fails on editor state

.claude/settings.local.json is gitignored via the user's global gitignore
(~/.config/git/ignore), not this repo's .gitignore. oxfmt honours
.gitignore files but not the global excludes file, so it scanned the file and
pnpm format --check failed on something that is not part of the repo.

Added .claude/ to ignorePatterns in .oxfmtrc.json.

I checked whether the other tool-state directories need the same treatment by
dropping a deliberately mis-formatted file into each and running the check.
They do not — .turbo/, coverage/ and **/dist/ are all in this repo's
.gitignore and were skipped; a control file in a non-ignored directory was
correctly flagged. So .claude/ was the only gap.

Verified before (fails on .claude/settings.local.json) and after (clean).

2. CI now covers the published Node floor

packages/entity declares engines: { node: ">=20" }, but the matrix was
'["", "22.19"]' — the floor was declared and never tested. CONTRIBUTING.md
documented the gap and offered two ways out; this takes the first: "20" is
added to node-versions.

Also updated the two places that still described the gap: the "Node versions"
section of CONTRIBUTING.md and the explanatory comment in ci.yml.

Caveat, stated plainly: GitHub Actions cannot run locally, so this PR does
not prove the package builds and tests green on Node 20 — it only makes CI
check the claim for the first time. If the Node 20 job fails on this PR, that
is a real finding about the declared floor and should be addressed on its own
terms (fix the incompatibility, or raise engines to match what CI proves).
It should not be papered over by dropping 20 back out of the matrix.

3. The construction seal's declaration-emit coupling — and a correction

The review asked for a comment in packages/entity/tsconfig.json recording
that declaration must stay false in the extended base, per the CtorKey
comment in src/types.ts. That premise turns out to be wrong, so I recorded
the measured behaviour instead of the claimed one:

  • @btravstack/tsconfig/base.json sets declaration: true and
    declarationMap: true, not false. It also sets noEmit: true, so the
    tsc pass emits nothing either way.
  • Forcing declaration emit on this project (--declaration --emitDeclarationOnly), including with an exported entity subclass in
    src/, produces no TS4020 on TypeScript 5.9.3 or 7.0.2. TypeScript emits
    a ProbeOrg_base alias and Sealed is exported, so the private name is
    nameable.

The constraint is real, but it lives one level out: a consumer that exports
an entity subclass with declaration: true does hit it against the published
d.ts. Reproduced on both compilers with the built package:

error TS4020: 'extends' clause of exported class 'Organization'
              has or is using private name 'BaseInstance'.
error TS4020: 'extends' clause of exported class 'Organization'
              has or is using private name 'CtorKey'.

So the comment I added points at what actually matters: tsdown reads this
tsconfig for its --dts emit, which is what shapes the published declarations,
and the seal's private names surface as TS4020 in consumers that re-export a
subclass. It also flags that the src/types.ts comment is stale.

Two follow-ups worth their own issues, deliberately not done here:

  • The CtorKey comment in src/types.ts should be rewritten — it attributes
    the constraint to a tsconfig setting that is not set the way it says.
  • The consumer-facing TS4020 is an undocumented limitation of the seal: any
    library author re-exporting an entity subclass with declaration emit on is
    blocked. Exporting Sealed / BaseInstance from the public types would fix
    it, but that is a design change, not a cleanup.

No settings were changed.

4. equals across sibling subclasses is now specified

equals tests other instanceof Base, where Base is the class produced by
one Entity(...) call — so two bare subclasses of the same entity compare
equal when their data matches. Defensible, but the README only said "the same
entity type", which did not settle it.

Added a sentence to the equals bullet in "Instance members" stating the rule
(identity is the entity a class was built from, not the class itself), and a
test in equality.spec.ts pinning the sibling-subclass case alongside the
existing "different entity types are unequal" test.

Changeset

Included (patch). Items 1–3 are repo tooling and carry no user-facing change,
but item 4 changes the published README — the documented contract a consumer
reads — so it belongs in the release notes even though the behaviour it
describes is unchanged.

Gate

All green locally: pnpm format --check, pnpm lint, pnpm typecheck,
pnpm test (62 tests, 8 files), pnpm knip, pnpm build.

…couplings

- oxfmt: ignore .claude/ so `format --check` stops failing on editor state
  that is gitignored globally but not by this repo's .gitignore
- ci: add Node 20 to the matrix so the package's engines floor is proven
- tsconfig: record how the construction seal reaches the published d.ts
- docs: state the equals rule for sibling subclasses, with a test
Copilot AI lite review requested due to automatic review settings August 6, 2026 17:12

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.

Comment thread .github/workflows/ci.yml Outdated
@btravers
btravers merged commit 3e7d8b0 into main Aug 6, 2026
@btravers
btravers deleted the chore/review-cleanups branch August 6, 2026 19:08
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