Skip to content

Code review fixes: registration, lazy proxies, build task, diagnostics - #2

Open
autonomous-low-noise wants to merge 16 commits into
dfenerski:masterfrom
autonomous-low-noise:fix/code-review-findings
Open

Code review fixes: registration, lazy proxies, build task, diagnostics#2
autonomous-low-noise wants to merge 16 commits into
dfenerski:masterfrom
autonomous-low-noise:fix/code-review-findings

Conversation

@autonomous-low-noise

@autonomous-low-noise autonomous-low-noise commented Jul 11, 2026

Copy link
Copy Markdown

Fixes for the findings of a full code review of the library. One commit per fix; every behavioral fix ships with a self-verifying regression scenario in test/sandbox (run via the new run-regression.sh). All 18 scenarios (7 pre-existing + 11 new) pass, and tsc --noEmit is clean over the sandbox + synced lib.

Correctness fixes (each was empirically reproduced first)

  1. Registration honors explicit @Inject tokens — a constructor param typed as an interface/primitive/undecorated class crashed at import time even when @Inject('TOKEN') made it resolvable at settlement.
  2. Token → proxy stability — an override no longer orphans proxies handed out by settleLazy; the existing proxy & context are reused, so held proxies keep following the token.
  3. Transparent dependency proxy — added set/has/deleteProperty/ownKeys/getOwnPropertyDescriptor/getPrototypeOf traps, correct receiver, and instance-bound methods (with identity cache). Getters/methods using #private state no longer throw; Object.keys no longer leaks dependencyInstance; the __isUI5DependencyProxy brand now exists at runtime.
  4. Build task — rewrites every @Injectable() occurrence (was: first per file) with path+index-derived tokens (was: basename, colliding across folders in the global symbol registry).
  5. Cycle detection — constructor cycles now raise Dependency cycle detected: A -> B -> A instead of a bare stack overflow; undefined paramtypes (circular imports) get a descriptive error.
  6. Factory settlement side effects — settle(token, factory) no longer eagerly settles constructor deps whose instances the factory branch discarded.

Semantics & diagnostics

  1. Precedence read live at comparison time — @Precedence above @Injectable was silently ignored (stale snapshot in the wraplet); all class-level decorators are now order-insensitive, matching the seal/factory flags.
  2. Split-brain override warning — overriding a settled, unsealed token still proceeds (precedence design; @Seal opts out) but now logs which token/classes are involved.
  3. @Inject on method params throws instead of corrupting constructor token metadata.
  4. Final is actually final — Precedence.FINAL was 999, beatable by @Precedence(1000); now Number.MAX_SAFE_INTEGER.
  5. Errors carry context — container misses name the token, metadata misses name the class, factory-only violations name the class.
  6. reset() exported for consumer test isolation.
  7. Container dump logs at info (was error); typo fixes (injecitonToken, README's broken utliService example, missing settle import); prepublish → prepublishOnly (the former hasn't run on npm publish since npm 7).

Notes for the maintainer

  • package/dist is committed but stale relative to these fixes — worth a rebuild (npm run build) before the next publish; prepublishOnly now guarantees it going forward.
  • package.json's "types": "dist/index.d.ts" still looks stale given the README wires consumers to src/ via paths — left untouched deliberately.
  • Not run here: eslint and the karma suite (no node_modules for the library package in this environment). The sandbox suite + typecheck cover all changed code paths.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc

Dimitar Fenerski and others added 16 commits July 11, 2026 08:28
Dimitar Fenerski @claude
fix: honor explicit @Inject tokens during registration
64b3912 
register() recursed into every design:paramtypes entry, so a constructor
param typed as an interface (emitted as Object), a primitive, or any
undecorated class crashed at import time with E_REFLECT_UTIL_NO_DI_TOKEN —
even when the param carried an explicit @Inject token that settle() would
resolve via container lookup. Skip recursion for params with an explicit
token, mirroring the settlement logic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: reuse proxy & context on token override so lazy proxies survive
a9a2a97 
register() built a fresh proxy + context on every registration. When a
token was overridden after settleLazy() had handed out a proxy, the old
context was never hydrated: the settler callback settled the new wraplet
while the held proxy stayed dead forever. Overrides now reuse the
existing proxy & context, establishing the invariant that token -> proxy
is stable for the container's lifetime and only the context's instance
pointer changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: make the dependency proxy a transparent view of the instance
074c54d 
The proxy trapped only `get` and passed the proxy itself as receiver:

- writes/`in`/`delete`/`Object.keys` hit the internal proxy context
  (a pre-hydration write silently vanished after hydration and
  Object.keys leaked ['dependencyInstance'])
- getters and methods touching #private state threw ("Cannot read
  private member") even after hydration, because `this` was the proxy

The proxy now behaves as an empty object before hydration (reads yield
undefined, writes are ignored) and as a transparent view afterwards:
set/has/deleteProperty/ownKeys/getOwnPropertyDescriptor/getPrototypeOf
forward to the instance, getters receive the instance as receiver, and
methods are bound to the instance (with a cache to keep function
identity stable). The __isUI5DependencyProxy brand now also exists at
runtime, matching the ClassInstanceProxy type.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix(task): rewrite every @Injectable() occurrence with a collision-fr…
a065481 
…ee token

The token-preservation task used String.replace with a string pattern,
which rewrites only the FIRST occurrence: a file with two or more
polymorphic injectables shipped the rest with class-name tokens that
terser then mangled — silent, production-only token collisions. The
token was also Symbol.for(<basename>), so same-named files in different
folders collided in the global symbol registry.

Match all @Injectable() occurrences (the @ anchor also stops accidental
hits on identifiers ending in "Injectable()"), and derive the token from
the full resource path plus an occurrence index so every class in every
file gets a distinct, minification-stable token.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: include token & class context in error messages
4c1ef52 
All three error constants were thrown bare, so every failure looked
identical regardless of which token or class caused it. The container
miss now names the token (via a new stringifyToken helper in misc),
the missing-metadata error names the class, and the factory-only
violation names the class.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: detect dependency cycles instead of overflowing the stack
dab2baa 
A constructor cycle (A needs B, B needs A) recursed in settle() until a
bare RangeError with no diagnostic. settle() now tracks in-flight tokens
on a settlement stack and reports the full chain on re-entry, e.g.
"Dependency cycle detected: CYCLE:A -> CYCLE:B -> CYCLE:A". The stack
unwinds in a finally block so failed settlements leave no residue.

Also guard register() against a paramtype of undefined (the class
reference was not initialized when the decorator ran, typically a
circular import) with a descriptive error naming the class and parameter
index, instead of a cryptic TypeError from the Reflect API.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: skip nested settlement when a dependency factory is provided
f3bd206 
settle(token, factory) on a non-factory-only class still settled every
constructor dependency, then discarded the results because the factory
branch never consumes them — instantiating and caching singletons nobody
asked for. Nested settlement now only runs when the constructor path
will actually receive the instances.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: read precedence from metadata at comparison time
528db40 
Precedence was snapshotted into the wraplet at registration, while the
seal & factory flags are read live from metadata. Since decorators apply
bottom-up, @precedence written above @Injectable ran after registration:
it updated the metadata but the container kept comparing against the
stale snapshot, silently ignoring the declared precedence. Precedence is
now read from metadata during override comparison like the other flags,
making all class-level decorators order-insensitive; the redundant
wraplet field is dropped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: warn when overriding a settled dependency
3a3abaa 
An unsealed, settled token could be silently overridden by an equal-or-
higher-precedence registration, splitting the singleton: consumers that
had already settled kept the old instance while later settles produced a
new one, with no trace of why. The override still proceeds (that is the
precedence design; @seal is the opt-out), but it now logs a warning
naming the token and both classes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: reject @Inject on non-constructor parameters
8e71c1d 
Applied to a method parameter (possible via a type cast, or from plain
JavaScript), @Inject wrote the token into the CONSTRUCTOR parameter
metadata for that index, silently corrupting injection for the class.
The decorator now throws a descriptive error when propertyKey is
defined, i.e. whenever it does not decorate a constructor parameter.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: make Final precedence actually final
35c0225 
Precedence.FINAL was 999, so any @precedence(1000) silently out-ranked a
@Final() class. FINAL is now Number.MAX_SAFE_INTEGER: no user-supplied
precedence can exceed it (an equal value still cannot displace a settled
+ sealed dependency, and ties intentionally follow last-registration
semantics like any equal-precedence pair).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
feat: add reset() for test isolation
5b96efb 
The Injector is a static global, so consumers had no way to isolate unit
tests: registrations and settled singletons leaked across test cases.
reset() clears the container and the in-flight settlement stack; class
metadata is untouched, so re-decorating or re-importing registers anew.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix: log container dump at info level
282e58f 
dumpContainerInfo is a diagnostic aid, not an error condition; logging
it via console.error polluted error monitoring and test output.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
chore: fix typos in code & README usage example
27cd24b 
- InjectorUtil: injecitonToken -> injectionToken
- README (both copies): the BusinessService example declared utliService
  but used this.utilService (broken as written), and the controller
  snippet used settle without importing it

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
fix(package): use prepublishOnly so the build runs on npm publish
ab1c59d 
The deprecated prepublish script has not run during "npm publish" since
npm 7, so a publish could silently ship a stale dist and README.
prepublishOnly is the supported hook for exactly this purpose.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Dimitar Fenerski @claude
feat(test): add sandbox regression runner
00bc411 
One-shot runner for all sandbox scenarios (the pre-existing ones plus
the regression tests added alongside each fix), reporting pass/fail per
file. Scenarios are self-verifying via node:assert.

Usage: cd test/sandbox && npm run sync && npm i && ./run-regression.sh

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCrV3GrDLmJHehiNuKYByc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@autonomous-low-noise