Skip to content

Commit 43214e3

Browse files
kburov-scclaude
andcommitted
Address review: keep zero-arg public factory calls working
Before PR #19 the public Container.factories map held memoized factories that carried a `thisArg`, so calling `c.factories.svc()` directly (without going through `get()`) would still resolve dependencies through the container. After the chain switch the factory body uses call-site `this`, which means a direct invocation makes `this` the factories map itself — `this.get(...)` then throws for any service with dependencies. Fix in `buildFlatFactories`: when materializing the public flat view, wrap each factory in a tiny forwarder that calls the underlying memoized factory with the container as `this`. Memo state stays in the underlying factory's closure (so memoization is preserved and shared across the chain), and both `get()` and a direct `c.factories[token]()` now route through the same container — matching the old public contract. Adds a test that exercises a dependent service through the public factories map. 97/97 tests pass, 100% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1a96aef commit 43214e3

2 files changed

Lines changed: 20 additions & 1 deletion

File tree

src/Container.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,18 @@ export class Container<Services = {}> {
300300

301301
private buildFlatFactories(): Factories<Services> {
302302
const flat = Object.create(null) as Factories<Services>;
303+
const self = this;
303304
chainedForEach<Memoized<() => unknown>>(this.factoriesChain, (k, v) => {
304-
(flat as Record<string, Memoized<() => unknown>>)[k] = v;
305+
// Wrap each factory so direct invocation via `c.factories[token]()` resolves the
306+
// service through THIS container, not through the factories map (which would
307+
// otherwise be `this` inside the underlying memoized closure and break any
308+
// factory that calls `this.get(...)`). Forwarding to `v.call(self)` preserves
309+
// memoization — memo state lives in `v`'s closure and is shared across the chain
310+
// — and keeps `get()`'s call-site `this` semantics intact since `get()` and a
311+
// direct call both end up routing through `self`.
312+
const bound = (() => v.call(self)) as Memoized<() => unknown>;
313+
bound.delegate = v.delegate;
314+
(flat as Record<string, Memoized<() => unknown>>)[k] = bound;
305315
});
306316
return flat;
307317
}

src/__tests__/Container.spec.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,15 @@ describe("Container", () => {
576576
});
577577

578578
describe("when accessing factories", () => {
579+
test("direct invocation of factories[token] works for services with dependencies", () => {
580+
// Pre-PR-#19, memoized factories carried `thisArg`, so calling a factory directly
581+
// (bypassing `get()`) still resolved its dependencies through the container.
582+
// Consumers using the public `factories` map should not see `this.get is not a
583+
// function` from a dependent service.
584+
const c = Container.providesValue("dep", 42).provides("svc", ["dep"] as const, (d: number) => d * 2);
585+
expect(c.factories.svc()).toBe(84);
586+
});
587+
579588
test("Object.keys returns every registered token regardless of chain depth", () => {
580589
// Public `factories` exposes a flat own-property view; internal chain extension via
581590
// Object.create stays an implementation detail.

0 commit comments

Comments
 (0)