fix(schema, type): avoid ParseError on duplicate scope names and decouple parseCache - #1654
Conversation
e7a4ae7 to
4adba5c
Compare
There was a problem hiding this comment.
Important
Relaxing scope-name uniqueness silently breaks @ark/type's name-keyed parse cache. Worth addressing before merge.
Reviewed changes
@ark/schemascope registration — removed the duplicate-namethrowParseError, replaced the name-derived anonymous counter with a monotonicanonymousScopeCount, and now register/overwritescopesByName[this.name]unconditionally soarkArrayre-evaluation (dual-package hazard / HMR) no longer crashes.@ark/attestpackaging — movedarktypefromdependenciestopeerDependencies(>=2.0.0) with adevDependenciesentry for the workspace, and dropped the unused@ark/schemadependency.- Tests — added
@ark/schemaandarktypetests asserting two scopes named"Array"can coexist without throwing. pnpm-lock.yaml— regenerated for the attest dependency reshuffle.
ℹ️ Nitpicks
ark/schema/scope.ts:153-154—scopesByNameis now entirely write-only dead state (the throw it previously guarded was its only reader). Once the parse-cache keying in the main comment is decoupled from scope names, this map and thenameproperty become pure diagnostics and could be removed or documented as such.ark/attest/package.json:55—peerDependencies: "arktype": ">=2.0.0"admits future 3.x majors, which will break attest's directimport { type } from "arktype"reliance on arktype internals.^2.0.0is more honest about the semver contract (and the author's own test matrix only exercises 2.x).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
ℹ️ Incremental delta is clean — the prior
@ark/schemafinding remains open.
Reviewed changes
- Reverted
@ark/attestpackaging change — commit4adba5c6drops theark/attest/package.jsonandpnpm-lock.yamledits from the prior revision, narrowing this PR to the@ark/schemascope-collision fix only. This resolves the peer-dependency nitpick from the previous review (the"arktype": ">=2.0.0"peer-range concern is no longer part of the diff).
The remaining schema change and its two tests are unchanged from the prior review. The inline finding on ark/schema/scope.ts:278 (the @ark/type parse-cache keyed by scope name, in ark/type/parser/definition.ts:69) is unaffected by this commit and remains open.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
4adba5c to
8a87cd8
Compare
…uple parseCache by scope identity - Avoid throwing ParseError when multiple scopes share the same name (e.g. built-in 'Array' scope re-evaluated during HMR, multi-package monorepos, or dual-package instances) - Key parseCache by BaseScope instance via WeakMap instead of scope name to prevent cache collision across same-named scopes and avoid memory leaks - Use sequential counter for anonymous scopes and remove dead scopesByName map - Add unit tests verifying scope name tolerance and parseCache isolation Closes arktypeio#1617
8a87cd8 to
d8c628f
Compare
|
@pullfrog review please |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Duplicate-scope-name tolerance in
@ark/schema— removed the write-onlyscopesByNameledger and its "already named"throwParseError, replacing anonymous-scope naming with a module-local monotonic counter. Verified via repo-wide grep thatscopesByName(andscope.namereads outside the constructor) had no other consumers. - Per-scope parse cache in
@ark/type—parseCachenow keys byBaseScopeinstance through aWeakMap<BaseScope, Record<string, InnerParseResult>>, decoupling parse results between same-named scopes (including the builtinArraykeyword scope triggered by@ark/attestdual-package/HMR reloads)and letting temporary scopes be GC'd.ctx.$is always the owningBaseScope(createParseContextstamps it), so noWeakMapkey mismatch ins anyparseInnerDefinitioncall path. - Regression tests — new unit tests in both
@ark/schemaandarktypesimulate two scopes named"Array"(the builtin keyword scope name),asserting construction no longer throws; thearktypetest additionally pins thattype("a")resolves independently per scope. Both new tests fail against the pre-fix code (construction throw; name-keyed cache returning the other scope's"a"node)and pass now.pnpm tscand the scoped mocha suites clear (the only 2failingentries arethe known-unrelatedark/attestsnapPopulationnoise whenark/attest/outisn't built).
The root-cause framing in the issue (#1617) isčorrect: fixing @ark/attest's dependency pin alone wouldn't survive HMR re-evaluation, so making scope construction tolerant of duplicate names and keying parseCache by scope identity is the right depth of fix.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
Thanks @ssalbdivad :) Btw, I'm happy to help more on the PR/issue queue if that’s useful. Just say the word! |

Resolves #1617 and unblocks yamcodes/arkenv#895
Summary
In
@ark/schema,BaseScopepreviously threwA Scope already named ${this.name} already existsif a scope was registered with a name that was already recorded inscopesByName.Because ArkType's built-in
arkArraykeyword module is initialized at module evaluation time with{ name: "Array" }, any dual-package hazard (e.g.@ark/attestrunning alongside another version ofarktypein a monorepo) or Next.js/Vite HMR module re-evaluation would throw an uncatchableParseError: A Scope already named Array already existsduring startup.This PR:
throwParseErrorin@ark/schema/scope.tsand uses a sequential monotonic counter (anonymousScopeCount) for anonymous scopes (dropping the deadscopesByNamestate).parseCachein@ark/type/parser/definition.tsfrom scope names by keying onBaseScopeidentity using aWeakMap<BaseScope, Record<string, InnerParseResult>>. This ensures parse results are isolated per scope instance even if two scopes share the same name, and prevents memory leaks when temporary scopes are garbage-collected.@ark/schemaandarktypeasserting that scopes sharing the same name do not throw, and thatparseCachedoes not leak between same-named scope instances.Why this approach over alternatives?
@ark/attest/package.json(e.g.peerDependenciesor bumping versions)?Only changing
package.jsondoes not solve the root bug. Any time two slightly different versions ofarktypecoexist in a monorepo, or when a file is re-evaluated during dev-server HMR (Vite / Next.js), the built-inArraymodule would still throw a fatal startup crash. ChangingpeerDependenciesalso alters the installation contract for existing@ark/attestusers and churns lockfiles.WeakMapforparseCacheinstead of scope names?When multiple scopes share the same name, string definitions parsed in one scope could leak into another. Keying the cache by
BaseScopeobject reference guarantees 100% parse isolation per instance, and allows temporary scopes to be cleanly garbage-collected without memory leaks.Verification
pnpm lint: passed with 0 warnings/errors.pnpm prChecks: passed completely locally (lint, buildRepo, tsc, mocha unit tests, V8 fast properties, integration tests, benchmark thresholds, and tsVersions tests: 1713 passing).