diff --git a/.changeset/clean-cloths-taste.md b/.changeset/clean-cloths-taste.md new file mode 100644 index 0000000..1b82148 --- /dev/null +++ b/.changeset/clean-cloths-taste.md @@ -0,0 +1,5 @@ +--- +"@webiny/di": patch +--- + +fix: multiple singleton implementations of the same abstraction now resolve correctly in minified/production builds diff --git a/.gitignore b/.gitignore index a14081e..982b670 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ node_modules dist coverage/** .idea +/.claude/settings.local.json diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..316a5f2 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,125 @@ +# @webiny/di - Agent Guide + +## What This Is + +A TypeScript dependency injection container for the Webiny ecosystem. Published as `@webiny/di` on npm. ESM-only, Node >= 22. + +## Architecture + +The DI system has five core concepts: + +1. **Abstraction** (`src/Abstraction.ts`) - A typed token (`symbol`-based) that represents an interface. Created with `new Abstraction("Name")`. Each instance gets a unique symbol, so two abstractions with the same name are still distinct. + +2. **Implementation** - A class bound to an abstraction via `createImplementation()` or `Abstraction.createImplementation()`. Metadata (abstraction token, dependencies) is stored on the class using `reflect-metadata`. + +3. **Decorator** - Wraps an existing implementation. Created with `createDecorator()`. The decoratee is always the **last** constructor parameter. Additional dependencies come before it. + +4. **Composite** - Aggregates multiple implementations of the same abstraction into one. Created with `createComposite()`. Typically takes `[Abstraction, { multiple: true }]` as a dependency. + +5. **Container** (`src/Container.ts`) - Manages registrations and resolves instances. Supports child containers (`createChildContainer()`), where child overrides are preferred during resolution but unresolved dependencies fall back to the parent. + +## Resolution Order + +When resolving a single abstraction from a container: + +1. Composite (if registered) +2. Instance registrations (`registerInstance`) - last registered wins +3. Class registrations (`register`) - last registered wins +4. Factory registrations (`registerFactory`) - last registered wins +5. Walk up to parent container and repeat +6. Throw if nothing found (unless `{ optional: true }`) + +Decorators are applied after resolution, in registration order. + +## Child Container Semantics + +Child containers resolve dependencies starting from the **requesting** container (the one `resolve()` was called on), not from where the registration lives. This means a service registered in a parent can have its dependencies overridden by a child. The `resolveFrom` parameter in internal methods tracks this origin. + +## Lifetime Scopes + +- **Transient** (default) - New instance on every `resolve()` call. +- **Singleton** - One instance per container where registered. Cached after first resolution (including decorators). Shared with all child containers that don't shadow the registration. + +## Key Files + +| File | Role | +| ------------------------ | -------------------------------------------------------------------------------- | +| `src/Container.ts` | All registration and resolution logic | +| `src/Abstraction.ts` | Token class with factory methods | +| `src/Metadata.ts` | `reflect-metadata` wrapper (keys prefixed `wby:`) | +| `src/types.ts` | Core types and advanced mapped types for dependency validation | +| `src/create*.ts` | Factory functions (`createImplementation`, `createDecorator`, `createComposite`) | +| `src/is*.ts` | Runtime type guards | +| `src/DependencyGraph.ts` | WIP, not used in production, excluded from coverage | + +## Type System + +The `Dependencies` type in `types.ts` maps constructor parameters to their abstraction declarations. It enforces at compile time that: + +- Required params map to `Abstraction` or `[Abstraction]` or `[Abstraction, { multiple: false }]` +- Optional params (`?`) require `[Abstraction, { optional: true }]` +- Array params require `[Abstraction, { multiple: true }]` + +Type-level tests live in `__tests__/types.test-d.ts`. + +## Toolchain + +| Tool | Purpose | +| ---------------- | ------------------------------------- | +| **pnpm** | Package manager (v10 in CI) | +| **TypeScript 6** | Type checking (`tsc --noEmit`) | +| **Vitest** | Test runner (v4, `--run --typecheck`) | +| **rslib** | Bundler (ESM, esnext, with DTS) | +| **oxlint** | Linter (NOT eslint) | +| **oxfmt** | Formatter (NOT prettier) | +| **changesets** | Versioning and publishing | + +## Commands + +```sh +pnpm test # run all tests with typecheck +pnpm lint # tsc + oxlint + oxfmt check +pnpm build # rslib build +pnpm test:coverage # V8 coverage for src/ +``` + +Run a single test file: `pnpm vitest run __tests__/container.test.ts` + +## Checks to Run Before Committing + +Run all three in order. All must pass — this matches CI. + +```sh +pnpm lint # tsc (type errors) + oxlint (lint) + oxfmt --check (formatting) +pnpm build # rslib build → dist/index.js + dist/index.d.ts +pnpm test # vitest --run --typecheck (46 tests + type-level tests) +``` + +If `oxfmt --check` fails, fix with `pnpm oxfmt --write `. Do not use prettier. + +## CI + +GitHub Actions on every push: `pnpm install --frozen-lockfile && pnpm lint && pnpm build && pnpm test`. + +## Test Structure + +- `__tests__/container.test.ts` - Core registration, resolution, decorators, composites, factories, error cases, type shorthand tests. +- `__tests__/singletons.test.ts` - Singleton lifetime across parent/child hierarchy. +- `__tests__/childContainer/` - Cross-container resolution bug tests with multi-level dependency override scenarios. +- `__tests__/types.test-d.ts` - Compile-time type assertion tests. +- `__tests__/setupEnv.ts` - Imports `reflect-metadata` globally for tests. + +## Conventions + +- No comments unless explaining a non-obvious "why". +- No eslint or prettier - use oxlint and oxfmt. +- `.js` extensions in imports (ESM resolution). +- Tests use `describe`/`test` from Vitest, not `it` (except `singletons.test.ts` which uses `it`). +- Abstractions are created as module-level constants (e.g., `const Logger = new Abstraction("Logger")`). +- Implementations are created via factory functions and stored as constants (e.g., `const ConsoleLoggerImpl = createImplementation({...})`). +- Run `pnpm changeset` before opening any PR that should trigger a release. + +## Known Issues + +- `DependencyGraph.ts` is WIP and uses `@ts-nocheck`. It references a `graphlib` dependency that isn't installed. Excluded from coverage. +- The child container test file (`__tests__/childContainer/childContainer.test.ts`) includes tests for a cross-resolution bug where child overrides must propagate through parent-registered services. This is a critical correctness property of the container. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..3e4cdf5 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,3 @@ +# @webiny/di + +See [AGENTS.md](./AGENTS.md) for full codebase documentation, architecture, conventions, and toolchain reference. diff --git a/__tests__/registry/abstractions.ts b/__tests__/registry/abstractions.ts new file mode 100644 index 0000000..fb347aa --- /dev/null +++ b/__tests__/registry/abstractions.ts @@ -0,0 +1,14 @@ +import { Abstraction } from "../../src/index.js"; + +export interface IPlugin { + name: string; + execute(): string; +} + +export interface IPluginRegistry { + getAll(): IPlugin[]; + executeAll(): string[]; +} + +export const PluginAbstraction = new Abstraction("Plugin"); +export const PluginRegistryAbstraction = new Abstraction("PluginRegistry"); diff --git a/__tests__/registry/implementations.ts b/__tests__/registry/implementations.ts new file mode 100644 index 0000000..2124103 --- /dev/null +++ b/__tests__/registry/implementations.ts @@ -0,0 +1,67 @@ +import { createImplementation } from "../../src/index.js"; +import { + PluginAbstraction, + PluginRegistryAbstraction, + type IPlugin, + type IPluginRegistry +} from "./abstractions.js"; + +export class AuthPlugin implements IPlugin { + name = "auth"; + + execute(): string { + return "auth:executed"; + } +} + +export class CachePlugin implements IPlugin { + name = "cache"; + + execute(): string { + return "cache:executed"; + } +} + +export class LoggingPlugin implements IPlugin { + name = "logging"; + + execute(): string { + return "logging:executed"; + } +} + +export class PluginRegistry implements IPluginRegistry { + constructor(private plugins: IPlugin[]) {} + + getAll(): IPlugin[] { + return this.plugins; + } + + executeAll(): string[] { + return this.plugins.map(p => p.execute()); + } +} + +export const AuthPluginImpl = createImplementation({ + abstraction: PluginAbstraction, + implementation: AuthPlugin, + dependencies: [] +}); + +export const CachePluginImpl = createImplementation({ + abstraction: PluginAbstraction, + implementation: CachePlugin, + dependencies: [] +}); + +export const LoggingPluginImpl = createImplementation({ + abstraction: PluginAbstraction, + implementation: LoggingPlugin, + dependencies: [] +}); + +export const PluginRegistryImpl = createImplementation({ + abstraction: PluginRegistryAbstraction, + implementation: PluginRegistry, + dependencies: [[PluginAbstraction, { multiple: true }]] +}); diff --git a/__tests__/registry/registry.test.ts b/__tests__/registry/registry.test.ts new file mode 100644 index 0000000..bdc36f4 --- /dev/null +++ b/__tests__/registry/registry.test.ts @@ -0,0 +1,78 @@ +import { describe, test, expect, beforeEach } from "vitest"; +import { Container } from "../../src/index.js"; +import { PluginAbstraction, PluginRegistryAbstraction } from "./abstractions.js"; +import { + AuthPlugin, + AuthPluginImpl, + CachePlugin, + CachePluginImpl, + LoggingPlugin, + LoggingPluginImpl, + PluginRegistryImpl +} from "./implementations.js"; + +describe("Plugin Registry - Singleton Scope", () => { + let container: Container; + + beforeEach(() => { + container = new Container(); + container.register(AuthPluginImpl).inSingletonScope(); + container.register(CachePluginImpl).inSingletonScope(); + container.register(LoggingPluginImpl).inSingletonScope(); + container.register(PluginRegistryImpl).inSingletonScope(); + }); + + test("registry is a singleton - resolving twice returns the same instance", () => { + const registry1 = container.resolve(PluginRegistryAbstraction); + const registry2 = container.resolve(PluginRegistryAbstraction); + + expect(registry1).toBe(registry2); + }); + + test("registry contains all registered plugin implementations", () => { + const registry = container.resolve(PluginRegistryAbstraction); + const plugins = registry.getAll(); + + expect(plugins).toHaveLength(3); + expect(plugins.some(p => p instanceof AuthPlugin && p.name === "auth")).toBe(true); + expect(plugins.some(p => p instanceof CachePlugin && p.name === "cache")).toBe(true); + expect(plugins.some(p => p instanceof LoggingPlugin && p.name === "logging")).toBe(true); + }); + + test("plugins inside the registry are the same singleton instances as individually resolved", () => { + const registry = container.resolve(PluginRegistryAbstraction); + const plugins = registry.getAll(); + + const allResolved = container.resolveAll(PluginAbstraction); + + for (const plugin of plugins) { + const matchingResolved = allResolved.find(p => p.constructor === plugin.constructor); + expect(plugin).toBe(matchingResolved); + } + }); + + test("plugin order matches registration order", () => { + const registry = container.resolve(PluginRegistryAbstraction); + const plugins = registry.getAll(); + + expect(plugins[0]).toBeInstanceOf(AuthPlugin); + expect(plugins[1]).toBeInstanceOf(CachePlugin); + expect(plugins[2]).toBeInstanceOf(LoggingPlugin); + }); + + test("child container resolves the same singleton registry as the parent", () => { + const child = container.createChildContainer(); + + const registryFromParent = container.resolve(PluginRegistryAbstraction); + const registryFromChild = child.resolve(PluginRegistryAbstraction); + + expect(registryFromParent).toBe(registryFromChild); + }); + + test("executeAll delegates to every plugin in order", () => { + const registry = container.resolve(PluginRegistryAbstraction); + const results = registry.executeAll(); + + expect(results).toEqual(["auth:executed", "cache:executed", "logging:executed"]); + }); +}); diff --git a/__tests__/singletonCacheKeyCollision/fixture.ts b/__tests__/singletonCacheKeyCollision/fixture.ts new file mode 100644 index 0000000..6495264 --- /dev/null +++ b/__tests__/singletonCacheKeyCollision/fixture.ts @@ -0,0 +1,93 @@ +import { Container, Abstraction, createImplementation } from "../../src/index"; + +interface IPlugin { + name: string; + execute(): string; +} + +interface IPluginRegistry { + getAll(): IPlugin[]; + executeAll(): string[]; +} + +const PluginAbstraction = new Abstraction("Plugin"); +const PluginRegistryAbstraction = new Abstraction("PluginRegistry"); + +class AuthPlugin implements IPlugin { + name = "auth"; + + execute(): string { + return "auth:executed"; + } +} + +class CachePlugin implements IPlugin { + name = "cache"; + + execute(): string { + return "cache:executed"; + } +} + +class LoggingPlugin implements IPlugin { + name = "logging"; + + execute(): string { + return "logging:executed"; + } +} + +class PluginRegistry implements IPluginRegistry { + constructor(private plugins: IPlugin[]) {} + + getAll(): IPlugin[] { + return this.plugins; + } + + executeAll(): string[] { + return this.plugins.map(p => p.execute()); + } +} + +const AuthPluginImpl = createImplementation({ + abstraction: PluginAbstraction, + implementation: AuthPlugin, + dependencies: [] +}); + +const CachePluginImpl = createImplementation({ + abstraction: PluginAbstraction, + implementation: CachePlugin, + dependencies: [] +}); + +const LoggingPluginImpl = createImplementation({ + abstraction: PluginAbstraction, + implementation: LoggingPlugin, + dependencies: [] +}); + +const PluginRegistryImpl = createImplementation({ + abstraction: PluginRegistryAbstraction, + implementation: PluginRegistry, + dependencies: [[PluginAbstraction, { multiple: true }]] +}); + +const container = new Container(); +container.register(AuthPluginImpl).inSingletonScope(); +container.register(CachePluginImpl).inSingletonScope(); +container.register(LoggingPluginImpl).inSingletonScope(); +container.register(PluginRegistryImpl).inSingletonScope(); + +const registry = container.resolve(PluginRegistryAbstraction); +const plugins = registry.getAll(); +const results = registry.executeAll(); + +const output = { + pluginCount: plugins.length, + names: plugins.map(p => p.name), + results, + distinctInstances: new Set(plugins.map(p => p.constructor)).size +}; + +process.stdout.write(JSON.stringify(output)); diff --git a/__tests__/singletonCacheKeyCollision/rspackMinified.test.ts b/__tests__/singletonCacheKeyCollision/rspackMinified.test.ts new file mode 100644 index 0000000..47b4620 --- /dev/null +++ b/__tests__/singletonCacheKeyCollision/rspackMinified.test.ts @@ -0,0 +1,98 @@ +import { describe, test, expect } from "vitest"; +import { rspack } from "@rspack/core"; +import { execFileSync } from "child_process"; +import path from "path"; +import fs from "fs"; +import os from "os"; +import { fileURLToPath } from "url"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +function bundle(outputDir: string): Promise { + return new Promise((resolve, reject) => { + const compiler = rspack({ + entry: path.resolve(__dirname, "fixture.ts"), + output: { + path: outputDir, + filename: "bundle.js" + }, + target: "node", + mode: "production", + resolve: { + extensions: [".ts", ".js"], + extensionAlias: { + ".js": [".ts", ".js"] + } + }, + module: { + rules: [ + { + test: /\.ts$/, + use: { + loader: "builtin:swc-loader", + options: { + jsc: { + parser: { syntax: "typescript" }, + transform: { + legacyDecorator: true, + decoratorMetadata: true + } + } + } + } + } + ] + }, + optimization: { + minimize: true, + mangleExports: true + } + }); + + compiler.run((err, stats) => { + if (err) return reject(err); + if (stats?.hasErrors()) { + return reject(new Error(stats.compilation.errors.map(e => e.message).join("\n"))); + } + resolve(); + }); + }); +} + +describe("Singleton cache key collision - rspack minified bundle", () => { + let outputDir: string; + let result: { + pluginCount: number; + names: string[]; + results: string[]; + distinctInstances: number; + }; + + test("bundle fixture with rspack and execute it", async () => { + outputDir = fs.mkdtempSync(path.join(os.tmpdir(), "di-minified-test-")); + + await bundle(outputDir); + + const bundlePath = path.join(outputDir, "bundle.js"); + expect(fs.existsSync(bundlePath)).toBe(true); + + const stdout = execFileSync("node", [bundlePath], { encoding: "utf-8" }); + result = JSON.parse(stdout); + }); + + test("registry should contain 3 plugins", () => { + expect(result.pluginCount).toBe(3); + }); + + test("each plugin should have a distinct name", () => { + expect(result.names).toEqual(["auth", "cache", "logging"]); + }); + + test("each plugin should execute with its own result", () => { + expect(result.results).toEqual(["auth:executed", "cache:executed", "logging:executed"]); + }); + + test("all 3 plugins should be distinct class instances", () => { + expect(result.distinctInstances).toBe(3); + }); +}); diff --git a/__tests__/singletonCacheKeyCollision/simulated.test.ts b/__tests__/singletonCacheKeyCollision/simulated.test.ts new file mode 100644 index 0000000..d672537 --- /dev/null +++ b/__tests__/singletonCacheKeyCollision/simulated.test.ts @@ -0,0 +1,61 @@ +import { describe, test, expect } from "vitest"; +import { Container, Abstraction, createImplementation } from "../../src/index.js"; + +describe("Singleton cache key collision - simulated minified class names", () => { + test("singleton implementations with identical class names resolve to distinct instances", () => { + interface IService { + id(): string; + } + + const ServiceAbstraction = new Abstraction("Service"); + + const ServiceA = { value: "a" }; + const ServiceB = { value: "b" }; + const ServiceC = { value: "c" }; + + function makeImpl(marker: { value: string }) { + class a implements IService { + id(): string { + return marker.value; + } + } + return a; + } + + const ImplA = makeImpl(ServiceA); + const ImplB = makeImpl(ServiceB); + const ImplC = makeImpl(ServiceC); + + expect(ImplA.name).toBe("a"); + expect(ImplB.name).toBe("a"); + expect(ImplC.name).toBe("a"); + + const RegA = createImplementation({ + abstraction: ServiceAbstraction, + implementation: ImplA, + dependencies: [] + }); + const RegB = createImplementation({ + abstraction: ServiceAbstraction, + implementation: ImplB, + dependencies: [] + }); + const RegC = createImplementation({ + abstraction: ServiceAbstraction, + implementation: ImplC, + dependencies: [] + }); + + const container = new Container(); + container.register(RegA).inSingletonScope(); + container.register(RegB).inSingletonScope(); + container.register(RegC).inSingletonScope(); + + const all = container.resolveAll(ServiceAbstraction); + + expect(all).toHaveLength(3); + expect(all[0]!.id()).toBe("a"); + expect(all[1]!.id()).toBe("b"); + expect(all[2]!.id()).toBe("c"); + }); +}); diff --git a/bugs/SINGLETON_CACHE_KEY_COLLISION.md b/bugs/SINGLETON_CACHE_KEY_COLLISION.md new file mode 100644 index 0000000..ab164b4 --- /dev/null +++ b/bugs/SINGLETON_CACHE_KEY_COLLISION.md @@ -0,0 +1,72 @@ +# Bug: Singleton cache key collision with minified class names + +## Summary + +When multiple implementations of the same abstraction are registered as singletons, and a bundler (rspack, webpack, esbuild) minifies class names, all implementations resolve to the first one registered. The singleton cache returns stale instances because the cache key is identical for all of them. + +## Root Cause + +`src/Container.ts` line 228 builds the singleton cache key as: + +```typescript +const instanceKey = `${abstraction.token.toString()}::${registration.implementation.name}`; +``` + +- `abstraction.token.toString()` is the same for all implementations of the same abstraction (e.g., `"Symbol(Plugin)"`). +- `registration.implementation.name` is the class name, which bundlers minify to the same short identifier (e.g., `"a"`). + +Result: all implementations get the same cache key (e.g., `"Symbol(Plugin)::a"`), and the first cached instance is returned for all subsequent resolves. + +## Reproduction + +Register three different classes with the same `class.name` as singletons against the same abstraction. Resolve all — only the first implementation is ever instantiated. + +```typescript +function makeImpl(marker: string) { + class a { + id() { + return marker; + } + } + return a; +} + +const ImplA = makeImpl("a"); // ImplA.name === "a" +const ImplB = makeImpl("b"); // ImplB.name === "a" +const ImplC = makeImpl("c"); // ImplC.name === "a" + +container + .register(createImplementation({ abstraction, implementation: ImplA, dependencies: [] })) + .inSingletonScope(); +container + .register(createImplementation({ abstraction, implementation: ImplB, dependencies: [] })) + .inSingletonScope(); +container + .register(createImplementation({ abstraction, implementation: ImplC, dependencies: [] })) + .inSingletonScope(); + +container.resolveAll(abstraction); +// Expected: [{ id: "a" }, { id: "b" }, { id: "c" }] +// Actual: [{ id: "a" }, { id: "a" }, { id: "a" }] +``` + +## Failing Test + +- `__tests__/singletonCacheKeyCollision/simulated.test.ts` — simulates minified class names +- `__tests__/singletonCacheKeyCollision/rspackMinified.test.ts` — bundles with real rspack and executes + +## Impact + +Any project using `@webiny/di` with a bundler that minifies class names will see this bug when registering multiple singleton implementations of the same abstraction. Single-implementation abstractions are unaffected. + +## Environment + +Works in development (class names preserved). Breaks in production builds with minification enabled. + +## Possible Fixes + +The cache key must be unique per registration, not per class name. Options: + +1. **Registration index** — include the registration's position in the array (e.g., `Symbol(Plugin)::0`, `Symbol(Plugin)::1`). Simple, but breaks if registrations are reordered. +2. **Unique ID per registration** — assign a unique symbol or counter to each `Registration` object at creation time. Stable regardless of class name or order. +3. **Use the registration object itself as key** — switch `instances` from `Map` to `WeakMap` or `Map`. No string key needed. diff --git a/bugs/SINGLETON_CACHE_KEY_COLLISION_FIX_SPEC.md b/bugs/SINGLETON_CACHE_KEY_COLLISION_FIX_SPEC.md new file mode 100644 index 0000000..d456390 --- /dev/null +++ b/bugs/SINGLETON_CACHE_KEY_COLLISION_FIX_SPEC.md @@ -0,0 +1,153 @@ +# Fix Spec: Singleton Cache Key Collision + +## Problem + +```typescript +// src/Container.ts line 228 +const instanceKey = `${abstraction.token.toString()}::${registration.implementation.name}`; +``` + +When rspack minifies class names, `AuthPlugin`, `CachePlugin`, and `LoggingPlugin` all become `a`. The cache key for all three becomes `Symbol(Plugin)::a`, so the first cached instance is returned for every resolve. + +## Proposed Fix + +Use the `Registration` object reference itself as the singleton cache key instead of a string derived from `class.name`. This is the approach used by inversify and tsyringe — object identity is guaranteed unique even after bundler minification. + +### Why object identity works + +The `Registration` object is created once in `register()` (line 41), stored in the `registrations` Map array (line 48), and the same reference is read back in: + +- `tryResolveFromCurrentContainer` (line 208): `const registration = registrations[registrations.length - 1]!` +- `resolveMultiple` (line 293): `for (const registration of registrations)` + +Both paths pass the same object reference to `resolveRegistration`. `Map` uses reference equality (`===`) for object keys, so the same `Registration` always hits the same cache entry. + +`inSingletonScope()` mutates `registration.scope` after creation, but mutating a property does not change object identity — the `Map` key still matches. + +### Changes + +#### 1. Change the `instances` map type + +```typescript +// src/Container.ts + +// Before: +private instances = new Map(); + +// After: +private instances = new Map(); +``` + +#### 2. Use the registration object as the cache key + +```typescript +// src/Container.ts — resolveRegistration() + +// Before: +const instanceKey = `${abstraction.token.toString()}::${registration.implementation.name}`; +if (registration.scope === LifetimeScope.Singleton) { + const existing = this.instances.get(instanceKey); + if (existing) { + return existing; + } +} +// ... +if (registration.scope === LifetimeScope.Singleton) { + this.instances.set(instanceKey, decoratedInstance); +} + +// After: +if (registration.scope === LifetimeScope.Singleton) { + const existing = this.instances.get(registration); + if (existing) { + return existing; + } +} +// ... +if (registration.scope === LifetimeScope.Singleton) { + this.instances.set(registration, decoratedInstance); +} +``` + +That's it. No new fields, no counters, no changes to `Registration` type. + +### What stays the same + +- `Registration` interface in `types.ts` — no changes +- `resolveInternal` — unchanged, it delegates to `tryResolveFromCurrentContainer` +- `tryResolveFromCurrentContainer` — unchanged, it picks the last registration and calls `resolveRegistration` +- `resolveMultiple` — unchanged, it iterates all registrations and calls `resolveRegistration` for each +- `applyDecorators` — unchanged, decorators are applied after instance creation +- `registerInstance` — unchanged, instance registrations don't use the singleton cache +- `registerFactory` — unchanged, factory registrations don't use the singleton cache +- `registerDecorator` — unchanged, decorators are not cached as singletons + +### Resolution flow (before vs after) + +#### Before (broken) + +``` +register(AuthPluginImpl).inSingletonScope() → reg0 = Registration { impl: a, name: "a" } +register(CachePluginImpl).inSingletonScope() → reg1 = Registration { impl: a, name: "a" } +register(LoggingPluginImpl).inSingletonScope() → reg2 = Registration { impl: a, name: "a" } + +resolveAll(PluginAbstraction): + reg0 → key = "Symbol(Plugin)::a" → cache miss → create AuthPlugin → cache it + reg1 → key = "Symbol(Plugin)::a" → cache HIT → return AuthPlugin ← WRONG + reg2 → key = "Symbol(Plugin)::a" → cache HIT → return AuthPlugin ← WRONG + +Result: [AuthPlugin, AuthPlugin, AuthPlugin] +``` + +#### After (fixed) + +``` +register(AuthPluginImpl).inSingletonScope() → reg0 = Registration { impl: a } +register(CachePluginImpl).inSingletonScope() → reg1 = Registration { impl: a } +register(LoggingPluginImpl).inSingletonScope() → reg2 = Registration { impl: a } + +resolveAll(PluginAbstraction): + reg0 → key = reg0 (object ref) → cache miss → create AuthPlugin → cache it + reg1 → key = reg1 (object ref) → cache miss → create CachePlugin → cache it + reg2 → key = reg2 (object ref) → cache miss → create LoggingPlugin → cache it + +Result: [AuthPlugin, CachePlugin, LoggingPlugin] +``` + +### Child container behavior + +No change. Child containers walk up to the parent's `resolveRegistration`, which holds the parent's `Registration` objects. Singletons are cached in the container where the registration lives, keyed by that registration's object reference. + +``` +Parent: register(AuthPluginImpl).inSingletonScope() → reg0 +Child: resolve(PluginAbstraction) + → child has no registration + → walks to parent + → parent.resolveRegistration(reg0) → instances.get(reg0) + → cache miss → create → instances.set(reg0, instance) + → return instance + +Parent: resolve(PluginAbstraction) + → parent.resolveRegistration(reg0) → instances.get(reg0) + → cache HIT → return same instance + +Both get the same singleton. ✓ +``` + +### Edge cases + +**Same class registered twice under the same abstraction:** + +```typescript +container.register(AuthPluginImpl).inSingletonScope(); +container.register(AuthPluginImpl).inSingletonScope(); +``` + +Before: both get key `Symbol(Plugin)::AuthPlugin` → same cached instance. +After: two distinct `Registration` objects → two separate singleton instances. + +This is the correct behavior — two registrations should produce two instances. If the user wanted one instance, they should register once. + +**Registration objects are never cloned:** + +The fix relies on object identity. If `Registration` objects were ever shallow-copied or spread into a new object, the cache key would break. This does not happen anywhere in the codebase — `register()` creates the object, stores it in an array, and the same reference is read back during resolution. diff --git a/docs/SINGLETON_CACHE_KEY_COLLISION_TESTS.md b/docs/SINGLETON_CACHE_KEY_COLLISION_TESTS.md new file mode 100644 index 0000000..dda6504 --- /dev/null +++ b/docs/SINGLETON_CACHE_KEY_COLLISION_TESTS.md @@ -0,0 +1,159 @@ +# Test Plan: Singleton Cache Key Collision + +Tests to harden the fix for the singleton cache key collision bug where bundler minification causes all implementations of the same abstraction to share a cache key. + +## Shared Helpers + +`__tests__/singletonCacheKeyCollision/helpers.ts` + +A `makeImpl(marker)` factory that creates classes with identical `.name` (simulating minification). Each class returns its marker from an `id()` method. Also exports a shared `ServiceAbstraction`. + +```typescript +export const ServiceAbstraction = new Abstraction("Service"); + +export function makeImpl(marker: string) { + class a implements IService { + id(): string { + return marker; + } + } + return a; +} +``` + +## Test Files + +### 1. `simulated.test.ts` (existing) + +Basic reproduction — three minified singletons, `resolveAll` returns distinct instances. + +### 2. `rspackMinified.test.ts` (existing) + +Real rspack production bundle. Bundles a fixture with minification, executes it, asserts the output. + +### 3. `withDecorators.test.ts` + +Register three minified singleton implementations. Add a decorator for the same abstraction that wraps the `id()` return value (e.g., `"decorated:"`). Resolve all. + +**Assertions:** + +- All 3 resolved instances are decorator instances +- Each decorated instance returns a distinct id: `"decorated:a"`, `"decorated:b"`, `"decorated:c"` +- Resolving twice returns the same decorated singleton instances (cache integrity) + +### 4. `withChildContainer.test.ts` + +Parent registers three minified singletons (a, b, c). Child overrides one (registers a new implementation with marker "b-override" for the same abstraction). + +**Assertions:** + +- Parent `resolveAll` returns 3 instances: a, b, c +- Child `resolveAll` returns 4 instances: a, b, c (from parent) + b-override (from child) +- Child `resolve` (single) returns b-override (last registered in child wins) +- Parent singletons are the same object references when accessed from either container +- Child's override is a different instance from the parent's b + +### 5. `withMixedScopes.test.ts` + +Register three minified implementations — first as singleton, second as transient, third as singleton. + +**Assertions:** + +- `resolveAll` twice: singleton instances (first, third) are the same references across both calls +- Transient instance (second) is a different reference on each `resolveAll` call +- All three return distinct markers on every call (no cross-contamination) + +### 6. `withComposite.test.ts` + +Register three minified singletons. Register a composite that takes `[ServiceAbstraction, { multiple: true }]`. Resolve the composite as a single instance. + +**Assertions:** + +- Resolved composite contains all 3 implementations +- Each implementation inside the composite has a distinct id +- Implementations inside the composite are the same singleton instances as individually resolved +- Resolving the composite twice returns the same composite instance (if registered as singleton) + +### 7. `withResolveWithDependencies.test.ts` + +Register three minified singletons. Use `container.resolveWithDependencies()` to instantiate an unregistered `Consumer` class that takes `IService[]` as a constructor parameter. + +**Assertions:** + +- Consumer receives all 3 distinct implementations +- Each implementation has a distinct id: a, b, c +- The implementations injected into the consumer are the same singleton instances as individually resolved + +### 8. `withDeepHierarchy.test.ts` + +Register three minified singletons in the root container. Create a chain: root → child → grandchild → greatGrandchild. + +**Assertions:** + +- `resolveAll` from every level returns the same 3 singleton instances (same object references) +- `resolve` (single, last-registered wins) from every level returns the same instance +- Adding a new minified singleton at the grandchild level: greatGrandchild sees 4 implementations, child sees 3, root sees 3 +- The grandchild-level singleton is a distinct instance from the root-level ones + +## Coverage Gaps + +Current coverage: 89.88% statements, 81.7% branches. The following lines are uncovered and should be addressed by the tests above or by additional general tests. + +### `Container.ts` + +| Lines | Method | What's missing | +| ------- | -------------------------------- | --------------------------------------------------------------------------------------- | +| 59-61 | `registerFactory` | Registration path — no test calls `registerFactory` and resolves via single `resolve()` | +| 212-216 | `tryResolveFromCurrentContainer` | Factory fallback — when no class or instance registration exists, falls back to factory | +| 302-306 | `resolveMultiple` | Factory iteration in `resolveAll` — factories are never tested with `multiple: true` | + +### `Abstraction.ts` + +| Lines | Method | What's missing | +| ----- | ------------------------------- | ------------------------------------------------------------------------------------------------ | +| 52-61 | `Abstraction.createComposite()` | Tests use the standalone `createComposite()` function, never the method on the Abstraction class | + +### Proposed additional tests + +#### 9. `withFactory.test.ts` (new — general coverage, not minification-specific) + +Register three factories via `registerFactory` for the same abstraction. Each factory returns an object with a distinct marker. + +**Assertions:** + +- `resolve` returns the last registered factory's result +- `resolveAll` returns results from all three factories +- Decorators are applied to factory-produced instances +- Factories in parent container are accessible from child container +- Factories work alongside class registrations and instance registrations in `resolveAll` + +This covers Container.ts lines 59-61, 212-216, and 302-306. + +#### 10. `withAbstractionCreateComposite.test.ts` (new — general coverage) + +Use `Abstraction.createComposite()` instead of the standalone `createComposite()` function. + +**Assertions:** + +- Composite created via `abstraction.createComposite()` resolves the same as one created via `createComposite()` +- Composite aggregates all registered implementations + +This covers Abstraction.ts lines 52-61. + +## File Structure + +``` +__tests__/singletonCacheKeyCollision/ + helpers.ts + fixture.ts + rspackMinified.test.ts + simulated.test.ts + withDecorators.test.ts + withChildContainer.test.ts + withMixedScopes.test.ts + withComposite.test.ts + withResolveWithDependencies.test.ts + withDeepHierarchy.test.ts + withFactory.test.ts + withAbstractionCreateComposite.test.ts +``` diff --git a/package.json b/package.json index 9492fc2..db13d5d 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,8 @@ "devDependencies": { "@changesets/cli": "^2.31.0", "@rslib/core": "^0.21.5", + "@rspack/core": "^2.0.3", + "@types/node": "^25.8.0", "@vitest/coverage-v8": "^4.1.6", "oxfmt": "^0.49.0", "oxlint": "^1.64.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a60956b..3aaea5c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -14,10 +14,16 @@ importers: devDependencies: '@changesets/cli': specifier: ^2.31.0 - version: 2.31.0 + version: 2.31.0(@types/node@25.8.0) '@rslib/core': specifier: ^0.21.5 version: 0.21.5(typescript@6.0.3) + '@rspack/core': + specifier: ^2.0.3 + version: 2.0.3(@swc/helpers@0.5.21) + '@types/node': + specifier: ^25.8.0 + version: 25.8.0 '@vitest/coverage-v8': specifier: ^4.1.6 version: 4.1.6(vitest@4.1.6) @@ -32,7 +38,7 @@ importers: version: 6.0.3 vitest: specifier: ^4.1.6 - version: 4.1.6(@vitest/coverage-v8@4.1.6)(vite@7.1.12) + version: 4.1.6(@types/node@25.8.0)(@vitest/coverage-v8@4.1.6)(vite@7.1.12(@types/node@25.8.0)) packages: @@ -883,6 +889,9 @@ packages: '@types/node@12.20.55': resolution: {integrity: sha512-J8xLz7q2OFulZ2cyGTLE1TbbZcjpno7FaN6zdJNrgAdrJ+DZzh/uFR6YrTb4C+nXakvud8Q4+rbhoIWlYQbUFQ==} + '@types/node@25.8.0': + resolution: {integrity: sha512-TCFSk8IZh+iLX1xtksoBVtdmgL+1IX0fC9BeU4QqFSuNdN/K+HUlhqOzEmSYYpZUVsLYcPqc9KX+60iDuninSQ==} + '@vitest/coverage-v8@4.1.6': resolution: {integrity: sha512-36l628fQ/9a/8ihy97eOtEnvWQEdqULQOJtcaxtoNq0G1w3Mxd4szSahOaMM9/NGyZ+hyKcMtIW/WIxq0XQViQ==} peerDependencies: @@ -1376,6 +1385,9 @@ packages: engines: {node: '>=14.17'} hasBin: true + undici-types@7.24.6: + resolution: {integrity: sha512-WRNW+sJgj5OBN4/0JpHFqtqzhpbnV0GuB+OozA9gCL7a993SmU+1JBZCzLNxYsbMfIeDL+lTsphD5jN5N+n0zg==} + universalify@0.1.2: resolution: {integrity: sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==} engines: {node: '>= 4.0.0'} @@ -1558,7 +1570,7 @@ snapshots: dependencies: '@changesets/types': 6.1.0 - '@changesets/cli@2.31.0': + '@changesets/cli@2.31.0(@types/node@25.8.0)': dependencies: '@changesets/apply-release-plan': 7.1.1 '@changesets/assemble-release-plan': 6.0.10 @@ -1574,7 +1586,7 @@ snapshots: '@changesets/should-skip-package': 0.1.2 '@changesets/types': 6.1.0 '@changesets/write': 0.4.0 - '@inquirer/external-editor': 1.0.3 + '@inquirer/external-editor': 1.0.3(@types/node@25.8.0) '@manypkg/get-packages': 1.1.3 ansi-colors: 4.1.3 enquirer: 2.4.1 @@ -1766,10 +1778,12 @@ snapshots: '@esbuild/win32-x64@0.25.12': optional: true - '@inquirer/external-editor@1.0.3': + '@inquirer/external-editor@1.0.3(@types/node@25.8.0)': dependencies: chardet: 2.1.1 iconv-lite: 0.7.2 + optionalDependencies: + '@types/node': 25.8.0 '@jridgewell/resolve-uri@3.1.2': {} @@ -2099,6 +2113,10 @@ snapshots: '@types/node@12.20.55': {} + '@types/node@25.8.0': + dependencies: + undici-types: 7.24.6 + '@vitest/coverage-v8@4.1.6(vitest@4.1.6)': dependencies: '@bcoe/v8-coverage': 1.0.2 @@ -2111,7 +2129,7 @@ snapshots: obug: 2.1.1 std-env: 4.1.0 tinyrainbow: 3.1.0 - vitest: 4.1.6(@vitest/coverage-v8@4.1.6)(vite@7.1.12) + vitest: 4.1.6(@types/node@25.8.0)(@vitest/coverage-v8@4.1.6)(vite@7.1.12(@types/node@25.8.0)) '@vitest/expect@4.1.6': dependencies: @@ -2122,13 +2140,13 @@ snapshots: chai: 6.2.2 tinyrainbow: 3.1.0 - '@vitest/mocker@4.1.6(vite@7.1.12)': + '@vitest/mocker@4.1.6(vite@7.1.12(@types/node@25.8.0))': dependencies: '@vitest/spy': 4.1.6 estree-walker: 3.0.3 magic-string: 0.30.21 optionalDependencies: - vite: 7.1.12 + vite: 7.1.12(@types/node@25.8.0) '@vitest/pretty-format@4.1.6': dependencies: @@ -2603,9 +2621,11 @@ snapshots: typescript@6.0.3: {} + undici-types@7.24.6: {} + universalify@0.1.2: {} - vite@7.1.12: + vite@7.1.12(@types/node@25.8.0): dependencies: esbuild: 0.25.12 fdir: 6.5.0(picomatch@4.0.4) @@ -2614,12 +2634,13 @@ snapshots: rollup: 4.60.3 tinyglobby: 0.2.16 optionalDependencies: + '@types/node': 25.8.0 fsevents: 2.3.3 - vitest@4.1.6(@vitest/coverage-v8@4.1.6)(vite@7.1.12): + vitest@4.1.6(@types/node@25.8.0)(@vitest/coverage-v8@4.1.6)(vite@7.1.12(@types/node@25.8.0)): dependencies: '@vitest/expect': 4.1.6 - '@vitest/mocker': 4.1.6(vite@7.1.12) + '@vitest/mocker': 4.1.6(vite@7.1.12(@types/node@25.8.0)) '@vitest/pretty-format': 4.1.6 '@vitest/runner': 4.1.6 '@vitest/snapshot': 4.1.6 @@ -2636,9 +2657,10 @@ snapshots: tinyexec: 1.1.2 tinyglobby: 0.2.16 tinyrainbow: 3.1.0 - vite: 7.1.12 + vite: 7.1.12(@types/node@25.8.0) why-is-node-running: 2.3.0 optionalDependencies: + '@types/node': 25.8.0 '@vitest/coverage-v8': 4.1.6(vitest@4.1.6) transitivePeerDependencies: - msw diff --git a/rslib.config.ts b/rslib.config.ts index 5c3d08d..a24713a 100644 --- a/rslib.config.ts +++ b/rslib.config.ts @@ -1,5 +1,8 @@ import { defineConfig } from "@rslib/core"; export default defineConfig({ + source: { + tsconfigPath: "./tsconfig.build.json" + }, lib: [{ format: "esm", syntax: "esnext", dts: true, bundle: true }] }); diff --git a/src/Container.ts b/src/Container.ts index 9e9935c..078b8c5 100644 --- a/src/Container.ts +++ b/src/Container.ts @@ -15,7 +15,7 @@ import { isDecorator } from "./isDecorator.js"; export class Container { private registrations = new Map(); private decorators = new Map(); - private instances = new Map(); + private instances = new Map(); private factories = new Map any)[]>(); private instanceRegistrations = new Map(); private composites = new Map(); @@ -225,9 +225,8 @@ export class Container { resolutionStack: Map, resolveFrom: Container ): T { - const instanceKey = `${abstraction.token.toString()}::${registration.implementation.name}`; if (registration.scope === LifetimeScope.Singleton) { - const existing = this.instances.get(instanceKey); + const existing = this.instances.get(registration); if (existing) { return existing; } @@ -254,7 +253,7 @@ export class Container { ); if (registration.scope === LifetimeScope.Singleton) { - this.instances.set(instanceKey, decoratedInstance); + this.instances.set(registration, decoratedInstance); } resolutionStack.delete(abstraction.token); diff --git a/tsconfig.build.json b/tsconfig.build.json new file mode 100644 index 0000000..92be185 --- /dev/null +++ b/tsconfig.build.json @@ -0,0 +1,7 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "rootDir": "./src" + }, + "include": ["src"] +} diff --git a/tsconfig.json b/tsconfig.json index 50adc41..66c6cd0 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,14 +1,17 @@ { "compilerOptions": { "lib": ["esnext"], + "types": ["vitest/globals", "node"], + "paths": { + "~/*": ["./src/*"] + }, "module": "preserve", "noEmit": true, "strict": true, "skipLibCheck": true, "isolatedModules": true, "resolveJsonModule": true, - "moduleResolution": "bundler", - "rootDir": "./src" + "moduleResolution": "bundler" }, - "include": ["src"] + "include": ["src", "__tests__"] }