Skip to content

Commit 0cf0d62

Browse files
MontanaYour Name
andauthored
Fix memoize re-invoking factories that return undefined (#22)
## Problem `memoize()` uses `typeof memo !== "undefined"` as its cache-hit check, so any factory whose result is `undefined` is silently re-invoked on **every** `container.get()` call. This breaks the documented run-once/singleton guarantee — notably for the side-effect initializer pattern the docs recommend with `run()` (e.g. `Injectable("initCache", ["request"], (req) => populateCache(req))`, whose factory returns nothing and therefore re-runs on every subsequent resolution). Repro: ```ts let calls = 0; const c = Container.provides("init", () => { calls++; return undefined; }); c.get("init"); c.get("init"); c.get("init"); // calls === 3, expected 1 ``` ## Fix Track invocation with an explicit `invoked` flag instead of inspecting the memoized value. The flag is set only *after* the delegate returns, deliberately preserving the existing retry-on-throw behavior that existing tests rely on. ## Tests - New `memoize.spec.ts`: undefined-result caching, throw-then-retry, `delegate`/`isMemoized` behavior - Container-level regression test in `Container.spec.ts` All tests pass with 100% coverage maintained; ESLint clean. Co-authored-by: Your Name <you@example.com>
1 parent 9616a63 commit 0cf0d62

3 files changed

Lines changed: 59 additions & 1 deletion

File tree

src/__tests__/Container.spec.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,15 @@ describe("Container", () => {
397397
const container: Container<{ TestService: string }> = new Container({} as any);
398398
expect(() => container.get("TestService")).toThrowError('Could not find Service for Token "TestService"');
399399
});
400+
401+
test("a factory returning undefined is only invoked once", () => {
402+
const factory = jest.fn().mockReturnValue(undefined);
403+
const containerWithService = Container.provides("TestService", factory);
404+
405+
expect(containerWithService.get("TestService")).toBeUndefined();
406+
expect(containerWithService.get("TestService")).toBeUndefined();
407+
expect(factory).toHaveBeenCalledTimes(1);
408+
});
400409
});
401410

402411
describe("when getting the Container Token", () => {

src/__tests__/memoize.spec.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import { isMemoized, memoize } from "../memoize";
2+
3+
describe("memoize", () => {
4+
test("invokes the delegate only once and returns the cached result", () => {
5+
const delegate = jest.fn().mockReturnValue("value");
6+
const memoized = memoize(delegate);
7+
8+
expect(memoized()).toBe("value");
9+
expect(memoized()).toBe("value");
10+
expect(delegate).toHaveBeenCalledTimes(1);
11+
});
12+
13+
test("invokes the delegate only once even when it returns undefined", () => {
14+
const delegate = jest.fn().mockReturnValue(undefined);
15+
const memoized = memoize(delegate);
16+
17+
expect(memoized()).toBeUndefined();
18+
expect(memoized()).toBeUndefined();
19+
expect(delegate).toHaveBeenCalledTimes(1);
20+
});
21+
22+
test("does not cache when the delegate throws, allowing a retry", () => {
23+
const delegate = jest
24+
.fn()
25+
.mockImplementationOnce(() => {
26+
throw new Error("first call fails");
27+
})
28+
.mockReturnValue("recovered");
29+
const memoized = memoize(delegate);
30+
31+
expect(() => memoized()).toThrowError("first call fails");
32+
expect(memoized()).toBe("recovered");
33+
expect(delegate).toHaveBeenCalledTimes(2);
34+
});
35+
36+
test("exposes the original function via delegate and is detected by isMemoized", () => {
37+
const delegate = () => 42;
38+
const memoized = memoize(delegate);
39+
40+
expect(memoized.delegate).toBe(delegate);
41+
expect(isMemoized(memoized)).toBe(true);
42+
expect(isMemoized(delegate)).toBe(false);
43+
});
44+
});

src/memoize.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,15 @@ export function isMemoized(fn: unknown): fn is Memoized<AnyFunction> {
1010
}
1111

1212
export function memoize<Fn extends AnyFunction>(delegate: Fn): Memoized<Fn> {
13+
// Track invocation with a flag rather than checking `memo` against `undefined`, so that
14+
// factories which legitimately return `undefined` are still only invoked once. The flag is
15+
// set only after `delegate` returns, preserving the existing behavior of retrying on throw.
16+
let invoked = false;
1317
let memo: any;
1418
const memoized = function (this: any, ...args: any[]) {
15-
if (typeof memo !== "undefined") return memo;
19+
if (invoked) return memo;
1620
memo = delegate.apply(this, args);
21+
invoked = true;
1722
return memo;
1823
};
1924
memoized.delegate = delegate;

0 commit comments

Comments
 (0)