Selectors return shared mutable values

[elliottkember]

Hi there!

I think I've uncovered a typing bug with this library, where selectors return shared mutable copies of their datasets.

Basically, if you return Immutable state directly from redux the result is immutable (the store can't be mutated, which is good). But if you return a mutable version of that array (for example, filtering store data) a mutable copy is returned.

This is problematic because that mutable copy is memoized and then used for any other usages of that selector. So one component can break the state of all the components using that selector with the same inputs!

Here is a reproduction sandbox: https://codesandbox.io/p/sandbox/amazing-curran-cfvj53?file=%2Fsrc%2FApp.js%3A16%2C32

We were able to make a patch fix on our end that resolved this issue quite nicely for us, by changing Result to Immutable<Result>.

diff --git a/node_modules/reselect/es/index.d.ts b/node_modules/reselect/es/index.d.ts
index 944961d..b1e087c 100644
--- a/node_modules/reselect/es/index.d.ts
+++ b/node_modules/reselect/es/index.d.ts
@@ -3,6 +3,49 @@ export type { Selector, GetParamsFromSelectors, GetStateFromSelectors, OutputSel
 import { defaultMemoize, defaultEqualityCheck, DefaultMemoizeOptions } from './defaultMemoize';
 export { defaultMemoize, defaultEqualityCheck };
 export type { DefaultMemoizeOptions };
+
+/** Imported from Immer to avoid having an implicit dependency in this patch */
+type AnyFunc = (...args: any[]) => any
+type PrimitiveType = number | string | boolean
+type AtomicObject = Function | Promise<any> | Date | RegExp
+export type IfAvailable<T, Fallback = void> =
+	// fallback if any
+	true | false extends (T extends never
+	? true
+	: false)
+		? Fallback // fallback if empty type
+		: keyof T extends never
+		? Fallback // original type
+		: T
+type WeakReferences = IfAvailable<WeakMap<any, any>> | IfAvailable<WeakSet<any>>
+export type WritableDraft<T> = {-readonly [K in keyof T]: Draft<T[K]>}
+export type Draft<T> = T extends PrimitiveType
+	? T
+	: T extends AtomicObject
+	? T
+	: T extends IfAvailable<ReadonlyMap<infer K, infer V>> // Map extends ReadonlyMap
+	? Map<Draft<K>, Draft<V>>
+	: T extends IfAvailable<ReadonlySet<infer V>> // Set extends ReadonlySet
+	? Set<Draft<V>>
+	: T extends WeakReferences
+	? T
+	: T extends object
+	? WritableDraft<T>
+	: T
+export type Immutable<T> = T extends PrimitiveType
+	? T
+	: T extends AtomicObject
+	? T
+	: T extends IfAvailable<ReadonlyMap<infer K, infer V>> // Map extends ReadonlyMap
+	? ReadonlyMap<Immutable<K>, Immutable<V>>
+	: T extends IfAvailable<ReadonlySet<infer V>> // Set extends ReadonlySet
+	? ReadonlySet<Immutable<V>>
+	: T extends WeakReferences
+	? T
+	: T extends object
+	? {readonly [K in keyof T]: Immutable<T[K]>}
+	: T
+
 export declare function createSelectorCreator<
 /** Selectors will eventually accept some function to be memoized */
 F extends (...args: unknown[]) => unknown, 
@@ -20,16 +63,16 @@ export interface CreateSelectorFunction<F extends (...args: unknown[]) => unknow
     /** Input selectors as separate inline arguments */
     <Selectors extends SelectorArray, Result>(...items: [
         ...Selectors,
-        (...args: SelectorResultArray<Selectors>) => Result
-    ]): OutputSelector<Selectors, Result, (...args: SelectorResultArray<Selectors>) => Result, GetParamsFromSelectors<Selectors>, Keys> & Keys;
+        (...args: SelectorResultArray<Selectors>) => Immutable<Result>
+    ]): OutputSelector<Selectors, Immutable<Result>, (...args: SelectorResultArray<Selectors>) => Result, GetParamsFromSelectors<Selectors>, Keys> & Keys;
     /** Input selectors as separate inline arguments with memoizeOptions passed */
     <Selectors extends SelectorArray, Result>(...items: [
         ...Selectors,
-        (...args: SelectorResultArray<Selectors>) => Result,
+        (...args: SelectorResultArray<Selectors>) => Immutable<Result>,
         CreateSelectorOptions<MemoizeOptions>
-    ]): OutputSelector<Selectors, Result, ((...args: SelectorResultArray<Selectors>) => Result), GetParamsFromSelectors<Selectors>, Keys> & Keys;
+    ]): OutputSelector<Selectors, Immutable<Result>, ((...args: SelectorResultArray<Selectors>) => Immutable<Result>), GetParamsFromSelectors<Selectors>, Keys> & Keys;
     /** Input selectors as a separate array */
-    <Selectors extends SelectorArray, Result>(selectors: [...Selectors], combiner: (...args: SelectorResultArray<Selectors>) => Result, options?: CreateSelectorOptions<MemoizeOptions>): OutputSelector<Selectors, Result, (...args: SelectorResultArray<Selectors>) => Result, GetParamsFromSelectors<Selectors>, Keys> & Keys;
+    <Selectors extends SelectorArray, Result>(selectors: [...Selectors], combiner: (...args: SelectorResultArray<Selectors>) => Immutable<Result>, options?: CreateSelectorOptions<MemoizeOptions>): OutputSelector<Selectors, Immutable<Result>, (...args: SelectorResultArray<Selectors>) => Result, GetParamsFromSelectors<Selectors>, Keys> & Keys;
 }
 export declare const createSelector: CreateSelectorFunction<(...args: unknown[]) => unknown, typeof defaultMemoize, [equalityCheckOrOptions?: import("./types").EqualityFn | DefaultMemoizeOptions | undefined], {
     clearCache: () => void;

Other than the fact that it's a breaking change requiring a lot of readonly additions to codebases, I don't think there is any inherent risk here as these selector return values should not be modified. We found that a lot of our code had inconsistent types, depending on whether their selectors returned immutable values straight from the store, or filtered mutable values from selectors.

I will make this diff into a PR on the repo, but wanted to mark this as an issue beforehand to gauge reactions.
Edit: Just to note, I am using reselect v4 here as we have not migrated yet. I didn't see this as a breaking change in the "What's New in 5.0.0?" so it is likely still an issue.

[EskiMojo14]

I don't think this makes sense for reselect to be opinionated about, it just returns what you return. If you want to make sure it's readonly, use castImmutable from immer before returning.

[elliottkember]

I understand the desire for reselect to not be overly opinionated. The inconsistency between the props in our app is due to our use of immer through redux-toolkit which is outside the scope of reselect.

But the problem is that reselect is responsible for the memoized version of the object. If it were just returning the result of the selector function that would be one thing, but because this memoized value is shared between multiple usages of the selector it means that Reselect makes it shared mutable state.

It's not obvious that one would have to add castImmutable to each return statement in each selector to avoid sharing this mutable state. Is there a valid use case for returning shared mutable objects from selectors? I haven't been able to think of one.

Or alternatively – should this be a PR on redux-toolkit to replace their re-export of reselect with a readonly version?

[markerikson]

I'm not sure I understand what the actual complaint or "bug" is here.

Is it the general principle that "many calls to the same memoized function return the same reference", and thus an accidental mutation of that reference in one place could cause a problem elsewhere? Is it that "you shouldn't mutate a returned reference, but the types aren't readonly to enforce that"? Is it that there's some inconsistency in whether a returned type is wrapped in Immer's Immutable types based on whether it was a raw value straight from the store vs a new reference? What's the specific problem?

[elliottkember]

@markerikson Yes, sorry, in truth this is not really a reselect bug. Perhaps this would have been better phrased in the form of a suggestion. It sure seemed like a bug when I hit it!

The inconsistency factor is due to the combination of this library and redux-toolkit – the combination is common. I don't think the two libraries need to match each other, but Immer's types have definitely proven useful.

I think your first two principles are the thing. One shouldn't really mutate shared references because an accidental mutation could cause a problem, and a change to readonly would help people avoid that mistake.

The shared reference being mutable is unexpected, and it's not reactive so you don't find out right away. It makes sense once you think about it, but it's an easy mistake to make; we had code that called Array.reverse() inside an output selector, which mutated the input selector's cache. It was quite impossible to track down.

I can't think of a downside to immutable selector returns (other than the admittedly huge amount of churn that comes with a breaking change). I suspect most users of reselect are be at risk of mutating their memoized selector return value.

We could Object.freeze all the return values of every selector, but that feels like a lot of overhead for selectors.

Once I implemented this patch locally, many of our inconsistent props are consistent, our selectors are purer, and it feels much better to have all selected data immutable. It was a slightly tedious but rewarding refactor.

An alternative would be to make a wrapper called something like immutable-reselect. Personally, I would rather contribute directly to this library than create a new redux tool library. There are so many. But if needs must!

[EskiMojo14]

The inconsistency factor is due to the combination of this library and redux-toolkit – the combination is common. I don't think the two libraries need to match each other, but Immer's types have definitely proven useful.

I'm confused, where's the inconsistency? RTK doesn't wrap your state in Immutable either.

[akaltar]

RTK by default uses Immer's autoFreeze option, which prevents mistakes by freezing your objects which come out of the redux state, preventing accidental mutations in one component to affect the whole state.
Since reselect is often used with RTK like this, a lot of people assume that the reselect selectors also do this object freezing or propagate it. Of course in hindsight it's easy to see why it wouldn't.

I think this would be a great change that would prevent lots of bugs, and I can see at least 2 ways to go about it (Possibly both):

• What @elliottkember proposes, which is a typescript-level solution, that auto types the return value and relies on typescript to enforce this.
• Something like adding autoFreeze as a devOption to createSelectorCreator, which would cause all input/output selectors to be frozen, similar to Immer's autoFreeze, and let people opt in over time. This way you could avoid a major version bump. This would enforce it on the javascript level, benefiting even non-ts projects. But it would only trigger at run-time, while the typescript one could catch the issues at compile time

An aside but I've just uncovered a similar bug to what @elliottkember did, and the timing is eerie, it was also caused by Array.reverse() too!

Edited to add:
I see this as a very similar feature to input stability checks, where it's not strictly necessary, but greatly improves the DX, and preemptively catches bugs for you

[akaltar]

For anyone stumbling on this, I've realized you can manually implement the autofreeze behaviour centrally without patching reselect by taking one of the existing memoizer functions, and calling deepFreeze on the selector output before saving it to cache.
It's not ideal as you'll have to duplicate the sometimes not so simple memoizer function, and overriding it also disables the freezing, but it's what I've done in absence of official support for now.

@markerikson if there is appetite for integrating this safety feature directly, I'd love to create a PR

[EskiMojo14]

@akaltar that would work - here's a quick util to make it easy to wrap any memoizer:

import { lruMemoize, UnknownMemoizer } from 'reselect'
import { freeze } from 'immer'

function makeFreezingMemoizer<Memoizer extends UnknownMemoizer>(
  memoizer: Memoizer
) {
  return function wrapperMemoizer(...args) {
    const selector = memoizer(...args)
    return Object.assign(function wrapper(...args: any[]) {
      return freeze(selector(...args), true)
    }, selector)
  } as Memoizer
}

// for example
const freezingLruMemoize = makeFreezingMemoizer(lruMemoize)

[elliottkember]

It's been a few weeks without any updates to this thread. I still think this is a real gotcha, and I'm grateful for the contributions here where others have hit the same issue. It's not a bug by the strictest definition, but it definitely allows bugs like mine to occur in user-space.

Marking these selectors as immutable would help enforce a one-way data flow – unexpected shared mutable state is dangerous. Inadvertent mutations are very possible; I suspect many applications are mutating caches without realizing.

A TS Immutable type on returned values seems pretty low-risk and would help Typescript users avoid this mistake without any substantive logic change. My PR #738 is one way of solving this that has worked brilliantly for me so far – we no longer have any subtle non-reactive reordering bugs