Add modular multibindings: bind() + compose()#20
Open
kburov-sc wants to merge 13 commits into
Open
Conversation
Long .provides()..provides() chains caused container assembly ANRs on low-end devices because each call shallow-copied the entire factories map and the constructor walked every factory to rebind thisArg. With N services that's O(N²) work. This patch replaces the spread with prototype-chained extension (Object.create) and switches memoize to call-site `this`, eliminating the per-step rebinding pass. 8000-deep chain: 10286 ms -> 496 ms (~21x). All 87 tests pass with 100% coverage. See benchmarks/INVESTIGATION.md for the full write-up and benchmarks/provides-chain.ts (npm run bench) to reproduce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Internal chain-building callers (providesService, copy, provides(Container|PartialContainer)) always feed pre-memoized factories. Route them through a trusted private static factory that bypasses the constructor's per-step scan entirely. The public constructor keeps its backwards-compatible memoize-as-needed behaviour for raw user input. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PartialContainer.provides() had the same O(N²) spread pattern. Apply the same prototype-chain extension via Object.create on this.injectables. PartialContainer chain at 8000 services: 5199 ms -> 513 ms (~10x). Also: for...in and naive [k] lookup on a deep Object.create chain are themselves O(N²) in V8 (key deduplication). Introduce a chainedForEach helper that walks the chain in a single linear pass, and use it from all sites that traverse a potentially-chained map (Container.provides merge, PartialContainer.provides + getFactories + getTokens). The benchmark now also covers PartialContainer construction and materialization. Investigation doc updated with the new numbers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's format:check (prettier -l *) was failing on the changes from earlier commits. No functional changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The factories map was inheriting from Object.prototype, so c.get("toString")
on a container that hadn't registered the token would silently return the
result of Object.prototype.toString instead of throwing — a regression
introduced when we switched to Object.create-based chains. Root every
factory chain at a null-prototype object so token names that match
Object.prototype methods don't leak through get().
Add tests for four use cases that 100% line coverage didn't pin down:
- A service that declares $container as a dependency receives the
container that resolved it (including after a fork with override).
- Tokens that shadow Object.prototype methods (toString, hasOwnProperty,
constructor) resolve to their registered values, while unregistered
ones throw.
- A 100-deep linear dependency chain resolves to the correct values.
- copy(['token']) on a deeply chained container produces a fresh
memoization for the scoped service while non-scoped services stay
shared with the original.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR-introduced regression: switching from `{...this.factories}` to
`Object.create(this.factories)` made provides() construction O(1) per step
but turned every Container.get() into a prototype-chain walk costing
O(depth-of-token). V8's inline caches hide it; on Hermes (Snap mobile /
Composer, no JIT) the team measured a ~20x regression on a full key sweep
of an 800-deep chain.
Fix: materialize a flat own-property snapshot of `factories` on the first
get() and read from it thereafter. The factories chain itself stays
prototype-linked so provides() construction remains O(1). Container
instances are immutable after construction, so the cached snapshot is
valid for the container's lifetime.
Hot read path full sweep on V8 (Node): N=1600 drops from 13.3 ms/iter to
0.10 ms/iter (~129x). Curve is now linear in N. Construction numbers
unchanged (within ~10% noise).
Adds benchmarks/get-pass.ts + `npm run bench:get` to track this in CI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the separate `bench:get` script; `npm run bench` now runs both construction and read-path benchmarks back-to-back via benchmarks/index.ts. One command, complete perf picture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three regressions surfaced in PR review, all rooted in the Object.create-based chain leaking into externally observable behavior: 1. Container.factories observable shape changed. It was a flat own-property map; the chain switch made Object.keys(c.factories) return only the most recent layer. Fix: rename internal storage to `factoriesChain` (private) and expose `factories` as a lazy flat getter built on first access. External readers see the full set of tokens again; internal chain extension and the existing flat-cache lookup in get() continue to work unchanged. 2. Constructor skipped memoizing inherited factories. The slow path only scanned Object.keys(input), so `new Container(Object.create(parent))` left inherited raw factories un-memoized — breaking singleton semantics and crashing copy() (no .delegate to un-memoize). Fix: iterate via chainedForEach so own + inherited are both memoized into a clean null-rooted own-property root. 3. PartialContainer.__proto__ tokens disappeared. addInjectable did `Object.create(this.injectables)` where this.injectables inherited Object.prototype, so assigning to "__proto__" triggered the inherited setter and silently mutated the prototype. Fix: same null-rooted normalization as Container, plus a private withInjectables fast path that skips re-normalization for internal chain builders. Also null-rooted the temporary factories object built inside PartialContainer.getFactories so the materialized form preserves "__proto__" too. Adds one test per regression. 100% coverage holds at 96/96. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Pin down the public-contract properties the previous commit re-established so they can't silently regress: - get() and direct factories[token]() share memoization (single run). - Direct invocation works for a service inherited via a deep chain. - Direct invocation picks up dependency overrides applied later in the chain (matching the get() codepath and pre-PR thisArg behavior). 100/100 tests pass, 100% coverage held. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrite the two comments that anchored their explanation to "Pre-PR-#19" and "the pre-PR thisArg behavior" so each test describes the contract in terms of the current API. PR numbers age out; the test names already capture the property under test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…butions
`appendValue` / `appendClass` only work when one place owns the Container
chain. They don't scale when independent modules want to contribute to a
shared registry (plugins, middlewares, extension points) without coupling
through a single chain. Multibindings reify a contribution as a value —
modules export `Multibinding<S, D>`s, the wire-up site composes them.
API surface:
- `bind<S>()` returns a `MultibindingBuilder` against a registry shape `S`
(typically `typeof someContainer`). Methods:
- `contributeValue(token, value)` — eager literal
- `contributeClass(token, MyClass)` — InjectableClass with static deps
- `contribute(Injectable(...))` — pre-built InjectableFunction
- `withInternal(PartialContainer)` — private helpers not exposed on
the composed container's type
- `compose(core, ...bindings)` applies the bindings in order. A binding's
phantom `D` carries the union of its contributions' deps (minus what
`withInternal` provides); `compose` rejects calls where the core doesn't
satisfy `D`, surfacing the missing keys as a `missingDeps` field on the
offending binding rather than a generic type mismatch.
Implementation only uses public Container/PartialContainer APIs, so all
the perf-branch wins (O(1) chain extensions, prototype-rooted factories,
chainedForEach traversal, lazy flat-factories snapshot) flow through
automatically.
README gets a "Modular Multibindings" section: motivation, four-file
plugin example (registry / auth / metrics / wire-up), `withInternal`
privacy semantics with a worked dep-flow rule, and a when-to-use-which
table. "Key Concepts" picks up a `Multibinding` entry.
Tests + verification:
- 14 new tests, 100% coverage on Multibinding.ts.
- 4 `@ts-expect-error` guards pin missing-core-dep, missing-internal-dep,
non-array-token, and element-type-mismatch.
- Full suite: 114/114 pass at 100% coverage. tsc -b on cjs/esm/types and
`npm run lint` are clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…al()
Swap the fluent builder for a smaller value-oriented API:
multibindings(registry) — factory that captures the registry shape.
Has `.contribute(token, value | class)` and
`.contribute(injectable)` overloads. Also
callable as `multibindings<S>()` when only
a type is available.
combine(...mbs) — bundle several Multibindings into one;
deps union, applied left-to-right.
withInternal(partial, ...mbs) — non-positional private helpers; partial's
services are subtracted from every binding's
deps regardless of argument order.
compose(core, ...mbs) — unchanged.
The `Multibinding<S, D>` brand and `compose`'s `missingDeps` error semantics
are preserved.
Why the swap:
- Single-contribution modules (the common case) collapse from
`bind<Registry>().contributeClass("plugins", X).build()` to
`m.contribute("plugins", X)` — one verb, no builder type to learn.
- `withInternal` becomes non-positional. Previously, dep subtraction only saw
contributions chained after the `.withInternal(...)` call, so swapping the
order silently produced a binding with wrong deps. The new shape
`withInternal(partial, mb1, mb2, ...)` subtracts over the union of every
binding inside the call.
- Class vs Injectable contribute through one overloaded method that dispatches
at runtime on arg shape, rather than separate `contributeClass` / `contribute`
methods.
- Builder removed entirely — `multibindings()` returns a plain object with one
method. No `MultibindingBuilder` class, no per-`.contributeX` allocation.
Why the factory rather than a free `contribute<S>(...)`:
TypeScript doesn't allow partial type-arg application — `contribute<Registry>(token, X)`
would require providing every type param positionally, and adding defaults masks
the inference of `Class` from the runtime arg (the default fires before
inference, widening `D` to `Record<string, unknown>`). Routing through
`multibindings(registry)` (or `multibindings<S>()`) captures `S` once and lets
the remaining type params infer normally from runtime args.
Tests + verification:
- 19 tests, 100% coverage on Multibinding.ts.
- Four `@ts-expect-error` guards (missing core dep, missing internal dep,
non-array token, element-type mismatch, plus a Multibinding<S,D> annotation
round-trip).
- Full suite: 119/119 pass at 100% coverage. `npm run lint` and `tsc -b` clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Stacked on #19.
appendValue/appendClassonly work when one place owns the Container chain. This adds a value-based contribution shape so independent modules can extend a shared registry (plugins, middlewares, extension points) without coupling through a single chain.Change
bind<S>()→MultibindingBuilderwithcontributeValue/contributeClass/contribute(Injectable(...))/withInternal(PartialContainer)/build().compose(core, ...bindings)validates deps at compile time; missing keys surface as amissingDepsfield on the offending binding instead of a generic type mismatch.withInternallets a binding bring private helpers along; they're invisible to other bindings and don't widen the composed container's type.withInternalprivacy, and a when-to-use-which table.Test plan
npm test— 114 pass (14 new), 100% coverage held including new modulenpm run lint+tsc -b(cjs/esm/types) clean🤖 Generated with Claude Code