[utils] Port x-internals store improvements

[romgrk]

Part of the effort to deduplicate the store implementations between @mui/x-internals and @base-ui/utils, so MUI X packages can consume the Base UI store instead of shipping a parallel copy.

Ports the @mui/x-internals selector implementation into @base-ui/utils/store (supersedes the store changes from the refactor-store-merge branch, rebased onto current master):

• createSelector: support 7–8 input selectors (previously capped at 6).
• createSelectorMemoized: rewrap as createSelectorMemoizedWithOptions(options) factory exposing reselect memoize overrides (used by x-charts), and fix the single-combiner case to wrap an identity input selector instead of passing the bare combiner to reselect.
• Store.create(): restore the static factory used by MUI X call sites.
• Add Store.test.ts covering selector arity limits, memoization cache-key behavior, and extra-args passing.

Kept over the old branch: master's stricter set/update typings from #5423. Error messages are unchanged, so no new error codes.

With this released, @mui/x-internals/store can become a re-export of @base-ui/utils/store (plus its local useStoreEffect, which only uses the public store surface).

🤖 Generated with Claude Code

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@5489
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@5489
  

commit: b53610d

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	🔺+36B(+0.01%)	🔺+11B(+0.01%)

Details of bundle changes

Performance

Total duration: 900.01 ms -57.40 ms^(-6.0%) | Renders: 76 ⁽⁺⁰⁾ | Paint: 1,437.37 ms -88.74 ms^(-5.8%)

No significant changes — details

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	b53610d
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a95dcbe7ac8f30008b5066f
😎 Deploy Preview	https://deploy-preview-5489--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[michaldudak]

PR review

Two merge-blocking public API issues remain: the new Reselect options parameter is effectively untyped, and Store.create constructs the wrong class when inherited. The complete utils suite passed (114 tests), TypeScript passed, all PR checks are green, and focused runtime/type probes reproduced the findings below.

Bugs (4)

1. 🔴 The options factory exposes the wrong Reselect type

Location: packages/utils/src/store/createSelectorMemoized.ts:17

export const createSelectorMemoizedWithOptions =
  (options?: OverrideMemoizeOptions<UnknownMemoizer>): CreateSelectorFunction =>

OverrideMemoizeOptions describes arguments passed to a memoizer, not the options object accepted by createSelector. Combined with UnknownMemoizer, it becomes effectively any: unknown properties, primitives, and invalid nested values all compile.

Failure scenario: An argsMemoizeOptions value with equalityCheck: 123 compiles. The first selector call succeeds, but the second throws TypeError: equalityCheck is not a function.

Fix: Type this with an appropriately generic Reselect CreateSelectorOptions shape that preserves custom memoizer inference. Add negative type tests for unknown keys and invalid memoizer options.

2. 🔴 Store.create constructs the base class when inherited

Location: packages/utils/src/store/Store.ts:10

static create<T>(state: T) {
  return new Store(state);
}

Static methods are inherited, so ReactStore.create() and third-party subclass factories exist but always return a plain Store.

Failure scenario: ReactStore.create({ value: 1 }) is not an instance of ReactStore and has no context, selectors, or ReactStore methods.

Fix: Construct through a properly typed polymorphic this, or explicitly override/prevent the factory on subclasses. Add an inheritance regression test.

3. 🟠 The selector type accepts parameter shapes the runtime cannot support

Location: packages/utils/src/store/createSelector.ts:17

const Args extends any[],
const Selectors extends ReadonlyArray<Selector<any>>,

The public type does not enforce the documented maximum of seven input selectors and three fixed additional arguments, nor does it reject rest parameters despite dispatch relying on Function.length.

Failure scenario: A type-valid rest-parameter combiner such as (value, ...ids) => … receives an empty ids array. Four fixed extra arguments are silently truncated by createSelector, while the memoized version throws; eight input selectors also compile and then throw.

Fix: Bound selector and argument tuples to their runtime limits and reject rest-parameter combiners. Add compile-time boundary tests.

4. 🟡 A type-valid zero-argument memoized selector crashes directly

Location: packages/utils/src/store/createSelectorMemoized.ts:35

let cacheKey = state.__cacheKey__;

The new single-combiner handling allows createSelectorMemoized(() => 42) and types its result as a zero-argument function, but the implementation still assumes a state object.

Failure scenario: createSelectorMemoized(() => 42)() compiles and throws while reading __cacheKey__ from undefined.

Fix: Either require state in the single-combiner type or support state-less selectors with a stable sentinel cache key. Add a direct-call regression test.

Verdict

Request changes - the two newly exposed public APIs currently permit runtime failures and incorrect subclass construction.

---

🤖 Review generated with Codex

[romgrk]

All four findings addressed in 9f1f7e4:

1. Options typing — createSelectorMemoizedWithOptions is now generic over override memoizers and typed with Reselect's CreateSelectorOptions<typeof lruMemoize, typeof weakMapMemoize, …>. equalityCheck: 123 and unknown keys no longer compile; custom memoize/argsMemoize overrides keep their option inference. Negative type tests added in createSelector.spec.ts.

2. Store.create — now static create<T, This extends Store<T>>(this: new (state: T) => This, state: T): This, constructing new this(state). Inherited factories return proper subclass instances (ReactStore.create() works since its extra constructor params are optional). Covered by a subclass regression test and a ReactStore.create instanceof test.

3. Arity bounds — the type now rejects more than seven input selectors and more than three extra combiner arguments whenever the combiner's parameter count is statically known. One caveat: rest-parameter combiners produce an open-ended parameter tuple that is indistinguishable at the type level from a contextually-typed combiner (whose type falls back to the open ...Args constraint), so eagerly rejecting open tuples broke inference for valid untyped combiners. Open-ended tuples therefore pass the static check and remain covered by the runtime guards. Compile-time boundary tests added for the enforced limits.

4. Zero-argument memoized selector — the single-function form's result type now requires [state: object] when the combiner takes no parameters, so createSelectorMemoized(() => 42)() is a compile error while zero-param combiners invoked with the state (a real pattern in MUI X's virtualizer) keep working.

Verification: utils typecheck, lint, and 84 store tests pass here; the full mui-x monorepo also typechecks cleanly against the pkg.pr.new build of this commit (mui/mui-x#23335 is now pinned to it), which exercises ~80 selector call sites including the createSelectorMemoizedWithOptions usage in x-charts.

🤖 Generated with Claude Code

[michaldudak]

PR review

Reviewed at 18aa7b287, which already contains 9f1f7e4f5 — most of what that commit claims holds up. I confirmed the two real fixes with a standalone reselect 5.2.0 harness running this branch and master side by side: the single-combiner form was genuinely broken before (reselect got zero dependencies and called the combiner with no arguments), and the new arity-7/8 branches work. Nothing here is merge-blocking. Four medium issues come from behavior the diff changes on the way through — a removed runtime guard that turns a loud throw into silently dropped arguments, memoizeOptions clobbering the module's Object.is equality, and two type tightenings that break createSelector calls which compiled on master — plus two test gaps I verified by mutation. Verified locally with tsgo -b, the store suite, and @ts-expect-error probes compiled against both refs; I could not check MUI X call sites directly.

Bugs (5)

1. 🟠 Math.max(…, 0) removes the guard that caught combiners whose Function.length under-reports

Location: packages/utils/src/store/createSelectorMemoized.ts:38

const argsLength = Math.max(combiner.length - nSelectors, 0);

The clamp is needed for the zero-parameter combiner (createSelectorMemoized(() => 42)), but Function.length cannot distinguish that from a rest-parameter or wrapped combiner. When it under-reports, argsLength becomes 0, so no () => selectorArgs[i] input selector is wired up (line 58) and return fn(state) (line 110) omits the extra args from the call — so reselect also keys the whole selector on state alone. Verified against both refs:

const combiner = (s, ...extra) => s.value + (extra[0] ?? 0);
createSelectorMemoized(combiner)({ value: 1 }, 5);
// this branch: 1        (extra is [], and the result is cached against state alone)
// master:      throws 'Unsupported number of arguments'

This is worth flagging because the diff's own new type explicitly waves rest-parameter combiners through and defers to the runtime — createSelector.ts:31-33: "An open-ended parameter count cannot be validated statically (it occurs both for rest parameters and for combiners typed contextually), so it is left to the runtime guards." Line 38 is the runtime guard it is deferring to. The same tradeoff is described in the review-response comment above, but the guard being relied on is the one that was just relaxed.

Failure scenario: A consumer wraps a combiner for instrumentation (const wrapped = (...args) => fn(...args), .length === 0) or uses a rest signature. Every call returns the same wrong value regardless of the extra argument, with no error — where master threw immediately.

Fix: Reject rest-parameter combiners in ExtraArgsWithinLimit, on exactly the rationale NoOptionalParams already states ("memoization relies on Function.length"), and correct the comment at createSelector.ts:31-33. If runtime detection is also wanted, throw when combiner.length > 0 && combiner.length < nSelectors.

2. 🟠 Passing memoizeOptions silently discards the module's equalityCheck: Object.is

Location: packages/utils/src/store/createSelectorMemoized.ts:86-88

if (options) {
  reselectArgs = [...reselectArgs, options];
}

reselect combines creator options and call-site options with a shallow spread, so a caller-supplied memoizeOptions replaces { maxSize: 1, equalityCheck: Object.is } (lines 9-12) wholesale and lruMemoize falls back to ===. Verified:

// repeated NaN-valued input, two distinct state objects
createSelectorMemoizedWithOptions()(...)                              // combiner calls: 1  (Object.is)
createSelectorMemoizedWithOptions({ memoizeOptions: { maxSize: 2 } }) // combiner calls: 2  (===, NaN !== NaN)

This is the exact shape the new spec blesses at createSelector.spec.ts:78 (memoizeOptions: { resultEqualityCheck: … }).

Failure scenario: A consumer adds resultEqualityCheck to cut re-renders and instead loses Object.is as a side effect. Any state field that can be NaN — a ratio from a zero-height measurement, a parsed number — now misses the memo on every call, the combiner returns a fresh reference, and useStore's Object.is comparison re-renders on every store notification, the opposite of what they asked for.

Fix: Merge rather than replace — memoizeOptions: { maxSize: 1, equalityCheck: Object.is, ...options?.memoizeOptions }, same for argsMemoizeOptions — or narrow the accepted options to a Base UI-owned type that cannot clobber the baseline.

3. 🟠 ReactStore.create() returns a plain Store, dropping the ReactStore API

Location: packages/utils/src/store/Store.ts:13

static create<T, This extends Store<T>>(this: new (state: T) => This, state: T): This {
  return new this(state);
}

To be clear about the scope, because the comment above is largely right: at runtime create does construct the class it is called on, and for concrete subclasses the inferred type is correct too — I verified class MyStore extends ReactStore<State> {} gives MyStore.create(...) : MyStore with the full surface available, and the same for SubStore extends Store<…> and for ToastStore. That is presumably why the mui-x typecheck comes back clean.

The one case that does not hold is calling create directly on the generic class, where This is inferred from the this constraint and collapses to the base. Verified on 18aa7b287:

const store = ReactStore.create({ value: 1, label: 'a' });
expectType<Store<{ value: number; label: string }>, typeof store>(store); // holds — it is Store, not ReactStore
store.context;                    // error: Property 'context' does not exist on type 'Store<…>'
store.useSyncedValue('value', 2); // error: Property 'useSyncedValue' does not exist on type 'Store<…>'

That is exactly the call the two new assertions cover, and neither can detect it: ReactStore.test.tsx:18 uses toBeInstanceOf(ReactStore), which the runtime passes, and createSelector.spec.ts:103 asserts expectType<State, typeof store.state>, which holds for Store<State> just as well.

Two smaller things on the same signature: TooltipStore, PopoverStore, PreviewCardStore and DialogStore take three required constructor parameters, so create is a hard type error on all four; and ToastStore(initialState: InitialState) / FloatingRootStore(options: FloatingRootStoreOptions) satisfy the one-parameter shape with a value that is not the state, which reads oddly against the JSDoc line "Creates a store with the given initial state".

Failure scenario: Narrow — component code always goes through a concrete subclass. It bites someone who calls ReactStore.create({ open: false }) directly, then writes store.useSyncedValue('open', open) and gets a compile error on a method the class plainly has.

Fix: Tighten the assertions to expectType<ReactStore<State>, typeof store>(store) so the degradation is visible, then either accept the generic base class as a known limitation and say so in the JSDoc, or thread the full constructor parameter list. Note ConstructorParameters/InstanceType does not work as an alternative — I tried it, and it degrades ReactStore to ReactStore<object, unknown, …>.

4. 🟠 Two type tightenings break createSelector calls that compiled on master

Location: packages/utils/src/store/createSelector.ts:38-42, :72-74

? Parameters<Combiner> extends []
  ? // The state is still required for caching when the combiner ignores it.
    [state: object]

Both constraints are properties of createSelectorMemoized (which needs state.__cacheKey__ and dispatches through three fixed slots), but they live in the type both functions share. createSelector's single-function form returns the combiner verbatim (createSelector.ts:202), so neither applies to it. Confirmed by compiling the same probes against master and this head:

createSelector(() => 42)();                                 // master: ok    here: error
createSelector((s: S, a, b, c, d: number) => s.value + d);  // master: ok    here: error

Same root cause: Selectors['length'] is number for a non-tuple selector array, so createSelector(...deps, combiner) now resolves to the 'Up to seven input selectors are supported' string and fails too.

Failure scenario: A consumer upgrades and their createSelector(() => DEFAULT_STATE) call site stops compiling, even though the function it returns is unchanged and still takes no arguments. Nothing in this repo hits it (full tsgo -b is clean), so the cost lands on external consumers.

Fix: Split CreateSelectorFunction into the bounded/state-required memoized type and the unbounded one for createSelector, instead of parameterizing one type with BoundedSelectors — see Simplification 2. Also note [state: object] loses the state type entirely, so sel(someUnrelatedObject) compiles and writes __cacheKey__ onto it.

5. 🟡 The single-combiner memoized form recomputes on every store update

Location: packages/utils/src/store/createSelectorMemoized.ts:54

const selectors = inputs.length === 1 ? [(x: any) => x, combiner] : inputs;

The fix is correct — master really did hand reselect a single function with no dependencies — but the identity input selector makes the result cache key the state object itself, and Store.set/update/notifyAll all build { ...this.state, … }. So the memo misses on every update. Verified:

// state replaced with an unrelated field changed; the selected slice is untouched
createSelectorMemoized((s) => s.items.filter(i => i.active))          // combiner ran 2x, new reference
createSelectorMemoized((s) => s.items, (items) => items.filter(...))  // combiner ran 1x, same reference

Failure scenario: Someone reaches for createSelectorMemoized(combiner) because the name promises a stable result, subscribes with useStore, and re-renders on every unrelated store change. Not a regression, and the explicit two-argument spelling behaves identically, but nothing warns.

Fix: A JSDoc sentence stating that the single-combiner form is keyed on state identity and that stable results need separate input selectors (or a resultEqualityCheck).

Tests (5)

1. 🟠 The restructured createSelectorMemoized runtime paths survive deletion

Location: packages/utils/src/store/Store.test.ts:107

All six memoized tests use a combiner whose arity equals the input-selector count, so argsLength === 0 in every one. That leaves the case 1/2/3 reselectArgs construction, the () => selectorArgs[i] thunks, the new fallthrough write switch, and the fn(state, a1, a2, a3) dispatch entirely unexercised. Two mutations, both green at 36/36:

delete the whole `switch (argsLength) { case 3: fn.selectorArgs[2] = a3; … }` block  →  36 passed
revert `Math.max(combiner.length - nSelectors, 0)` to `combiner.length - nSelectors` →  36 passed

Failure scenario: The block this PR rewrote could be deleted outright and CI would stay green, so a future refactor of the selectorArgs plumbing has no safety net — and the deliberate clamp behavior change (Bug 1) is unpinned.

Fix: Add memoized cases at argsLength 1, 2 and 3 (e.g. createSelectorMemoized((s: S) => s.value, (v, x1, x2, x3) => …) called as sel(state, 1, 2, 3), asserting both the result and that a changed extra arg re-runs the combiner), plus createSelectorMemoized(() => 42)(state) === 42 and the argsLength > 3 throw.

2. 🟠 The new six-input-selector branch of createSelector is untested

Location: packages/utils/src/store/createSelector.ts:157

Store.test.ts:66 covers only the eight-argument branch (7 selectors + combiner). The seven-argument branch is fresh copy-paste and nothing reaches it. Mutation, green:

return g(va, vb, vc, vd, ve, vf, a1, a2, a3);
→      g(va, vb, vc, vd, ve, va, a1, a2, a3);   →  36 passed

Failure scenario: A wrong-variable slip in a hand-unrolled branch — the single most likely defect in this style of code — ships undetected.

Fix: Add a six-input-selector case alongside the existing seven-selector one, with distinct values per selector (powers of two, so any duplication changes the result).

3. 🟡 Only argsMemoize is exercised; memoize/memoizeOptions are not

Location: packages/utils/src/store/Store.test.ts:173

The one options test is genuine — I confirmed it fails when the forwarding block is removed — but it pins the argsMemoize path only. memoizeOptions is the path that silently drops Object.is (Bug 2), and it is the shape the type spec advertises.

Failure scenario: The equality-semantics loss in Bug 2 has no test that would catch it being introduced or fixed.

Fix: Add a memoizeOptions: { resultEqualityCheck } case asserting the returned reference is reused, and one pinning that Object.is semantics survive (repeated NaN input across two state objects → combiner called once).

4. 🟡 The create type assertions assert too little

Location: packages/utils/src/store/createSelector.spec.ts:103

expectType<State, typeof store.state>(store.state);

This holds for Store<State> just as well as ReactStore<State>, which is what let Bug 3 through. The sibling assertions at :92 and :99 correctly assert the instance type; this one does not. ReactStore.test.tsx:18-23 has the same gap at runtime — toBeInstanceOf plus state.value would pass even if construction were half-wired.

Failure scenario: A public factory ships with a degraded return type and a green suite.

Fix: expectType<ReactStore<State>, typeof store>(store), and assert store.context is {} in the runtime test.

5. 🟡 Selector tests live in Store.test.ts, store type tests live in createSelector.spec.ts

Location: packages/utils/src/store/Store.test.ts:45

AGENTS.md: "Each file/component is tested with the filename name.test.tsx… next to its source file." Store.test.ts holds describe('createSelector'), describe('createSelectorMemoized') and describe('createSelectorMemoizedWithOptions'), while createSelector.spec.ts:89-104 holds the Store.create/ReactStore.create type tests even though ReactStore.spec.ts already exists.

Failure scenario: pnpm test:jsdom createSelector --no-watch runs nothing, so the natural command for iterating on these two modules silently covers zero tests.

Fix: Split into createSelector.test.ts / createSelectorMemoized.test.ts, and move the create type assertions to Store.spec.ts / ReactStore.spec.ts.

Simplifications (4)

1. 🟡 The fallthrough switch is larger than the assignments it replaced and leaks a lint suppression

Location: packages/utils/src/store/createSelectorMemoized.ts:96-107

/* eslint-disable no-fallthrough */

switch (argsLength) {
  case 3:
    fn.selectorArgs[2] = a3;
  case 2:
    fn.selectorArgs[1] = a2;
  case 1:
    fn.selectorArgs[0] = a1;
  case 0:
  default:
}

I specifically hunted for the stale-slot read this replacement implies and there is none — reselectArgs only wires () => selectorArgs[i] for i < argsLength, so slots at or above argsLength are never read. The rewrite is behaviorally a no-op that saves at most two property writes on a fixed three-element array, immediately before a second switch on the same value. It costs more minified bytes than the three straight-line assignments, and the /* eslint-disable no-fallthrough */ is block-form with no matching enable, so it silences the rule for the remainder of the file.

Failure scenario: More bytes and a permanently disabled lint rule in a bundle-size-sensitive package, for an unmeasurable saving.

Fix: Restore the three unconditional assignments the base version had.

2. 🟡 The arity cap, its type machinery, and its runtime throw could all go away

Location: packages/utils/src/store/createSelector.ts:146-167

The two new branches duplicate the existing body verbatim (~50 lines) for arities nothing in this repo uses — createSelector and createSelectorMemoized have zero call sites outside packages/utils/src/store itself. The cap also produces a poor diagnostic, because the 'Up to seven input selectors are supported' string sits in the combiner slot:

error TS2345: Argument of type '(a: any, b: any, …) => any' is not assignable to
              parameter of type '"Up to seven input selectors are supported"'.
error TS7006: Parameter 'a' implicitly has an 'any' type.   (×8)

Nine errors, none pointing at the eighth selector. The spec works around this by annotating the combiner (createSelector.spec.ts:32), which suggests it was hit during authoring.

Failure scenario: ~50 lines of duplicated source, a bespoke type check and a runtime throw all exist to enforce a bound that has already moved once in this PR and that createSelectorMemoized escapes entirely — with a compiler error that blames the one argument that is correct.

Fix: Keep the unrolled branches for the arities actually used and add one generic fallback — combiner(...selectors.map((s) => s(state, a1, a2, a3)), a1, a2, a3). That removes the cap, ValidCombiner's selector-count arm and the 'Unsupported number of selectors' throw, and lets CreateSelectorFunction split into two plain types instead of carrying a BoundedSelectors boolean parameter (which surfaces to consumers as an unexplained CreateSelectorFunction<false> in hover tooltips and the emitted .d.ts).

3. 🟡 Store.create is a class static, so every consumer ships it

Location: packages/utils/src/store/Store.ts:13

@base-ui/utils sets sideEffects: false, so unused named exports from store/index.ts tree-shake out — but a class static does not. Every Base UI consumer that pulls in Store/ReactStore (combobox, select, tooltip, popover, toast, …) ships create for a constructor alias with no non-test callers in this repo.

Failure scenario: Bytes in every Base UI bundle for an API only MUI X calls.

Fix: If it is only needed downstream, a free createStore(state) function in a separate module tree-shakes where a static cannot.

4. ℹ️ Leftovers around the reworked argsLength

Location: packages/utils/src/store/createSelectorMemoized.ts:83, :117; packages/utils/src/store/createSelector.ts:28

After Math.max(…, 0) and the argsLength > 3 throw, argsLength is provably 0 | 1 | 2 | 3, so both default: throw arms are unreachable — turning case 3: into default: in each switch deletes them, including a minify-error-disabled string that survives minification. LengthOf is redundant because ExtraParams always resolves to a tuple, so its : number arm is dead. And const selectors = inputs.length === 1 ? … at :54 depends only on closure constants but is evaluated inside the cache-miss branch; hoisting it next to combiner also removes the inputs.length - 1 || 1 trick.

Docs (2)

1. 🟡 The two createSelectorMemoized* exports have no JSDoc at all

Location: packages/utils/src/store/createSelectorMemoized.ts:17, :125

createSelector carries a full JSDoc block with examples and constraints; its two siblings in the same published entry point carry none. Three things a consumer cannot discover from the code: that createSelectorMemoized accepts unlimited input selectors while createSelector caps at seven; that supplying memoizeOptions replaces the module's equality defaults (Bug 2); and that the single-combiner form is keyed on state identity (Bug 5).

Failure scenario: A consumer reads createSelectorMemoizedWithOptions in autocomplete, passes the reselect options they know, and gets different equality semantics than the module documents nowhere.

Fix: JSDoc on both exports covering the arity difference, the options-replacement semantics and the single-combiner caching caveat.

2. 🟡 Two comments in createSelector.ts describe guarantees that do not exist

Location: packages/utils/src/store/createSelector.ts:31-33, :73

 * ... so it is left to the runtime guards.

createSelector has no runtime guard on extra-argument count at all — it always passes a1, a2, a3, and a fourth extra parameter silently receives undefined — and createSelectorMemoized's guard is the one Math.max neutralizes for exactly the rest-parameter case the comment names (Bug 1). Separately:

? // The state is still required for caching when the combiner ignores it.
  [state: object]

Caching is a createSelectorMemoized concern; createSelector returns the combiner verbatim and caches nothing, so the comment justifies a constraint on the wrong function (Bug 4).

Failure scenario: A maintainer reading these comments concludes rest-parameter combiners are runtime-checked and that createSelector needs a state argument for caching — both false, and both would steer the fixes for Bugs 1 and 4 in the wrong direction.

Fix: Say that rest-parameter combiners are unvalidated in both directions (or reject them in the type), and scope the state-argument comment to the memoized variant once the type is split.

Verdict

Approve after nits - nothing blocking; the medium items are a silently-dropped-argument path, an equality default that callers can clobber unknowingly, and two mutation-verified test gaps on code this PR rewrote.

---

🤖 Review generated with Claude Code

[romgrk]

Review findings addressed in 94bb4d4. Summary per finding, including two deliberate divergences:

Bug 1 (runtime guard) — Restored: only a zero-length combiner may ignore its inputs (a deliberate pattern in MUI X's virtualizer); any other length below the selector count throws Unsupported number of arguments instead of silently dropping arguments. Static rest-parameter rejection was implemented and reverted with evidence of failure in both directions: a contextually-typed combiner falls back to the open ...Args tuple and is indistinguishable from a rest signature (the strict check rejected a legal multi-input memoized call), while DropFirst erases open tails behind fixed elements so (first, ...rest) behind two selectors passes the check anyway. The runtime guard is the enforcement point, pinned by tests, and the previously-misleading comment now says exactly this.

Bug 2 (options clobbering) — Object-form memoizeOptions now merge over { maxSize: 1, equalityCheck: Object.is }; a custom memoize or a bare equality function still replaces them (explicit whole-memoizer choices). Pinned by a NaN/Object.is test and a resultEqualityCheck reference-reuse test.

Bug 3 (Store.create on generic classes) — Kept as a documented limitation (your second fix option): the JSDoc now states that calling create on a generic base class constructs the right class at runtime but degrades the inferred type to Store, and the degradation is pinned explicitly in ReactStore.spec.ts. The runtime test asserts full wiring (context equals {}).

Bug 4 (type tightenings) — Fixed via the type split: CreateSelectorFunction and CreateSelectorMemoizedFunction are two plain types (no BoundedSelectors parameter). createSelector regains master parity — the single-function form is typed verbatim (createSelector(() => 42)() and the four-extra-args form compile again) — and its seven-input bound is written tolerantly, so a non-tuple selector list passes statically and hits the runtime throw rather than the misplaced combiner-slot error.

Bug 5 + Docs 1/2 — JSDoc added on both memoized exports (state-identity caching of the single-function form, options-merge semantics, Function.length constraints); both incorrect comments rewritten.

Tests 1–5 — Split per module (createSelector.test.ts, createSelectorMemoized.test.ts, plus matching spec files; the create type assertions moved to Store.spec.ts/ReactStore.spec.ts); Store.test.ts now tests Store behavior itself. Mutation gaps covered: extra-args dispatch at argsLength 1/2/3 with re-run-on-changed-arg assertions, the six-selector branch with distinct power-of-two values, the under-reporting throw, and both options paths. 101 tests passing.

Simplification 2 (drop the arity cap) — Not taken: createSelector keeps its unrolled fixed-arity dispatch and the seven-input cap; that monomorphic shape is the point of the function, and a generic fallback would silently deoptimize it. Unbounded input counts already have a home in createSelectorMemoized. The type split half of the suggestion is adopted, which also retires the nine-error diagnostic for the common cases.

Simplification 1 (fallthrough switch) — Not taken for the switch itself (kept intentionally); the leaked suppression is fixed with a matching eslint-enable scoped to the switch.

Simplifications 3/4 — Store.create stays (see Bug 3); selectors/nSelectors hoisted with the || 1 trick removed and the unreachable throw arms dropped where they were provably dead. LengthOf stays: indexing ['length'] on the unresolved conditional is a hard error (TS2536 + excessive type depth), which is why it exists.

Verification: utils typecheck/lint clean, 101 store tests passing, and all twelve store-consuming MUI X packages typecheck clean against the pkg.pr.new build of this commit (mui/mui-x#23335 is pinned to it).

🤖 Generated with Claude Code

[michaldudak]

PR review

The author’s response at 94bb4d41d resolves the earlier Store.create, object-form memoization, JSDoc, and test-coverage findings. One merge-blocking custom-memoizer mismatch remains, plus one non-blocking composed-selector type/runtime mismatch; the rest-parameter case is retained only as an informational limitation because the API explicitly documents rest combiners as unsupported. TypeScript, all 101 store tests, package build, publint, attw, and current CI pass, while focused typed runtime probes reproduce the remaining behavior below.

Bugs (3)

1. 🔴 A custom memoizer still receives lruMemoize options

Location: packages/utils/src/store/createSelectorMemoized.ts:40

const resolvedOptions =
  options !== undefined &&
  options.memoize === undefined &&

The author’s response says that a custom memoize replaces the module defaults. However, when memoize is overridden without memoizeOptions, this branch leaves the options unchanged. Reselect then shallow-merges them over the preconfigured creator, preserving { maxSize: 1, equalityCheck: Object.is } and passing that object to the custom memoizer.

Failure scenario: A type-valid custom memoizer whose optional second parameter defaults to an equality function receives the object instead. Calling the resulting selector throws TypeError: equalityCheck is not a function.

Fix: Explicitly reset memoizeOptions when overriding memoize without options, or construct a creator from fully resolved options. Add a custom-memoizer regression test.

2. 🟠 The composed createSelector form types an argument its runtime drops

Location: packages/utils/src/store/createSelector.ts:52

type ValidCombiner<Selectors extends ReadonlyArray<Fn>, Combiner extends Fn> =
  Selectors['length'] extends 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7
    ? NoOptionalParams<Combiner>

The author’s response correctly restores the single-function form verbatim, including its four-extra-argument example. This is a different path: once input selectors are composed, the new validation bounds only their count and never applies the documented three-extra-argument limit. Every composed runtime branch forwards only a1–a3.

Failure scenario: A composed selector with (value, a1, a2, a3, a4) compiles and is typed as accepting all four extras, but a4 is always undefined; a numeric combiner returns NaN.

Fix: Keep the single-function form verbatim, but apply the ExtraParams length check whenever input selectors are present. Add a compile-time boundary test and, ideally, a runtime guard for untyped callers.

3. ℹ️ Zero-length rest wrappers remain an unenforced limitation

Location: packages/utils/src/store/createSelectorMemoized.ts:63

const argsLength = combiner.length === 0 ? 0 : combiner.length - nSelectors;

The response deliberately permits zero-length combiners and explains why rest signatures cannot be rejected reliably at the type level. The public JSDoc also says rest combiners are unsupported, so this is not merge-blocking. The remaining caveat is that a type-valid rest-tuple wrapper also has runtime length zero, and extra caller arguments are silently discarded instead of producing the runtime error described as the enforcement point.

Failure scenario: A wrapper declared as (...args: [number, number]) => args[0] + args[1] compiles when composed with one input selector, but calling the result with its typed extra argument returns NaN.

Fix: If runtime enforcement is desired, detect extra arguments supplied to an argsLength === 0 output call and throw. Otherwise, document this zero-length ambiguity as part of the accepted rest-parameter limitation.

Verdict

Request changes - the custom-memoizer override still contradicts the stated replacement semantics and can crash; the remaining composed-selector issue is non-blocking.

---

🤖 Review generated with Codex

[romgrk]

Both fixed in 9481523.

Bug 1 (custom memoizer receives lruMemoize options) — You're right, and the JSDoc claim was wrong: the previous branch only rewrote the options when memoize was absent, so overriding memoize without memoizeOptions left reselect free to shallow-merge { maxSize: 1, equalityCheck: Object.is } into the custom memoizer's call. memoizeOptions is now explicitly cleared in that case, which reselect's memoizeOptions = [] destructuring default turns into "no options", so the custom memoizer falls back to its own. Supplying memoizeOptions alongside a custom memoize still forwards them verbatim (no merge with the lru defaults, since they describe a different memoizer), and the JSDoc now states this. Three regression tests: your crash scenario reproduced with a memoizer that defaults its second parameter to an equality function, an assertion that a custom memoizer receives no options at all, and one pinning verbatim forwarding. Mutation-checked — reverting the fix fails the first two.

Bug 2 (composed form types an argument the runtime drops) — Fixed as suggested: the extra-argument check is now shared between the two variants (ValidExtraArgs) and applies to createSelector whenever input selectors are present, while the single-function form stays verbatim and keeps its own signature. createSelector(input, (value, x1, x2, x3, x4) => …) is now a compile error; three extras still compile. Compile-time boundary tests added for both, plus one confirming the limit counts arguments after the selector results (two selectors + four extras). Mutation-checked — reverting the check makes the @ts-expect-errors unused.

I did not add the runtime guard you floated as optional for Bug 2. createSelectorMemoized throws because it already computes argsLength from Function.length for its slot wiring, so the check is free there; createSelector's unrolled branches never inspect the combiner, and adding an inspection purely to validate would put work into the construction path for a case the type now rejects. Happy to add it if you'd rather have the symmetry.

Bug 3 (zero-length rest wrappers) — Left as the documented limitation, per your assessment.

Verification: utils typecheck and lint clean, 104 store tests passing, no new error codes. Against the pkg.pr.new build of this commit, all twelve store-consuming MUI X packages typecheck clean and the x-charts suite (867 tests, the createSelectorMemoizedWithOptions consumer) passes — so no real call site exceeded the tightened composed-combiner bound. mui/mui-x#23335 is pinned to it.

🤖 Generated with Claude Code

[michaldudak]

PR review

The port behaves as described — I verified the single-combiner fix, the widened arity, and the reselect options plumbing at runtime, and none of them regress anything. Nothing here is merge-blocking; what follows is test coverage and cleanup worth a look, most notably that the test meant to pin the new memoizeOptions merge passes with the merge removed.

Tests (2)

1. 🟠 The memoizeOptions merge test passes with the merge removed

Location: packages/utils/src/store/createSelectorMemoized.test.ts:184

const state: S = { value: NaN };
selector(state);
selector(state);

expect(combiner).toHaveBeenCalledTimes(1);

Both calls pass the same state reference, so reselect's argsMemoize (weakMapMemoize) short-circuits on the arguments before the input selectors or the result memoizer run. The combiner is called once regardless of whether equalityCheck: Object.is survived the merge, so the stated assertion proves nothing — and the comment above it ("replacing the default equality with === would re-run the combiner") describes a mechanism that never executes.

I verified this by replacing the merge with resolvedOptions = options. The test does fail — but only because reselect's dev-only inputStabilityCheck emits a console warning that vitest-fail-on-console promotes to a failure. Adding devModeChecks: { inputStabilityCheck: 'never' } (as four neighbouring tests already do) or running under NODE_ENV=production makes it silently vacuous.

Failure scenario: Someone simplifies the option resolution and drops the { ...MEMOIZE_OPTIONS, ...memoizeOptions } merge. Object.is equality is silently replaced by ===, NaN state values stop memoizing for every consumer that passes memoizeOptions, and this test still passes.

Fix: Call the selector with a derived state so the cache key is shared and the result memoizer actually decides. I confirmed this version fails with the merge removed and passes with it:

const selector = createSelectorMemoizedWithOptions({
  memoizeOptions: { maxSize: 2 },
  devModeChecks: { inputStabilityCheck: 'never', identityFunctionCheck: 'never' },
})((state: S) => state.value, combiner);

const state: S = { value: NaN };
selector(state);
// A derived state shares __cacheKey__, so the reselect instance is reused and the
// memoize equality decides whether the combiner re-runs.
selector({ ...state });

expect(combiner).toHaveBeenCalledTimes(1);

2. 🟠 The single-combiner form with extra arguments has no runtime test

Location: packages/utils/src/store/createSelectorMemoized.test.ts:9

The headline fix wires the single-function form as [identity, ...argGetters, combiner]. The runtime tests cover only the zero-extra-arg shapes — (s) => ({ doubled: s.value * 2 }) and () => 42. The path where selectors.slice(0, -1) must yield the identity selector and the arg getters is exercised only by createSelectorMemoized.spec.ts:23, which is a type-level assertion and executes nothing.

That path is exactly what was broken before this PR: with selectors = [combiner], slice(0, -1) was empty and the combiner received (a1) instead of (state, a1). I confirmed it works now — createSelectorMemoized((s, a1) => s.value + a1) returns 15 for (state, 5) and memoizes correctly — but nothing pins it.

Failure scenario: A later refactor of the selectors construction reintroduces the old behavior for the single-combiner-with-args form. The combiner receives a1 as its state parameter, every selector of that shape returns garbage or throws, and the whole suite stays green.

Fix: Add a test alongside the existing single-combiner cases:

it('passes extra arguments to a single combiner alongside the state', () => {
  const combiner = vi.fn((s: { value: number }, a1: number) => s.value + a1);
  const selector = createSelectorMemoized(combiner);
  const state = { value: 10 };

  expect(selector(state, 5)).toBe(15);
  expect(selector(state, 5)).toBe(15);
  expect(combiner).toHaveBeenCalledTimes(1);

  expect(selector(state, 6)).toBe(16);
  expect(combiner).toHaveBeenCalledTimes(2);
});

Simplifications (2)

1. 🟡 The fallthrough switch replaces three unconditional writes without changing behavior

Location: packages/utils/src/store/createSelectorMemoized.ts:128

/* eslint-disable no-fallthrough */
switch (argsLength) {
  case 3:
    fn.selectorArgs[2] = a3;
  case 2:
    fn.selectorArgs[1] = a2;
  case 1:
    fn.selectorArgs[0] = a1;
  case 0:
  default:
}
/* eslint-enable no-fallthrough */

This replaces the previous three-line unconditional assignment. Only the first argsLength slots are ever read — the arg getters are built to match — so writing the unused tail slots was already harmless. The new form trades 3 lines for 12 plus a pair of lint directives, on a path that runs on every selector call, to save at most two array writes.

Failure scenario: Every future reader of this hot path has to work out that the fallthrough is intentional and that case 0 / default are deliberately empty, to conclude the behavior is identical to the three assignments it replaced.

Fix: Restore the unconditional form and drop both eslint directives:

fn.selectorArgs[0] = a1;
fn.selectorArgs[1] = a2;
fn.selectorArgs[2] = a3;

2. 🟡 Two more hand-unrolled arities for cold paths

Location: packages/utils/src/store/createSelector.ts:165

The 7- and 8-function branches add ~24 lines that repeat the existing pattern verbatim. The unrolling earns its place for the common 1–5 selector cases; a selector with 7 inputs is rare by construction, and neither branch is reachable from Base UI itself (createSelector has no callers outside packages/utils/src/store), so the bytes land in MUI X bundles for paths that will almost never run.

Failure scenario: The next arity bump copies the block again, and the cascade grows another ~12 lines per level, each an independent opportunity for an off-by-one in the h(va, vb, vc, vd, ve, vf, vg, a1, a2, a3) argument list.

Fix: Keep the unrolled branches for the hot low arities and collapse 6–7 selectors into one generic tail:

} else if (a && b && c && d && e && f && g) {
  const fns = [a, b, c, d, e, f, g, h].filter(Boolean) as Function[];
  const combine = fns.pop()!;
  selector = (state: any, a1: any, a2: any, a3: any) =>
    combine(...fns.map((fn) => fn(state, a1, a2, a3)), a1, a2, a3);
}

Verdict

Approve - everything above is a non-blocking test or cleanup nit; the ported behavior itself checks out.

---

🤖 Review generated with Claude Code

[romgrk]

Both test findings fixed in b53610d.

Test 1 (vacuous memoizeOptions merge test) — Correct, and the diagnosis was exact: both calls passed the same state reference, so argsMemoize short-circuited before the result memoizer ran. The test now calls the selector with a derived state ({ ...state }), which carries the same __cacheKey__ and therefore reuses the reselect instance, letting the memoize equality decide. I also disabled the dev-mode checks as you suggested, so a regression fails on the assertion rather than on vitest-fail-on-console. Mutation-checked: with resolvedOptions = options, it now fails with AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times — previously it only tripped the console warning. The misleading comment is rewritten to explain why a derived state is required.

Test 2 (single-combiner form with extra arguments) — Added the runtime test. Mutation-checked against the pre-PR behavior (const selectors = inputs): it fails with expected NaN to be 15, i.e. exactly the failure mode you described, where the combiner receives a1 in place of the state.

113 store tests pass; utils typecheck, lint and prettier clean.

On the two simplifications: I'm keeping the fallthrough selectorArgs switch and the unrolled 7/8-selector branches. Both are deliberate — the generic-tail variant was implemented earlier in this PR and reverted, since the point of createSelector here is the monomorphic unrolled dispatch, and unbounded input counts already have a home in createSelectorMemoized. Noting them as accepted trade-offs rather than oversights.

🤖 Generated with Claude Code