reselect icon indicating copy to clipboard operation
reselect copied to clipboard

`weakMapMemoize` with `resultEqualityCheck` is provided empty objects for first call.

Open keegan-lillo opened this issue 5 months ago • 3 comments

Hi! Thanks for all the great QOL improvements in Reselect 5, however I've found a pretty major issue with the resultEqualityCheck option when using the weakMapMemoize memoization. It calls the resultEqualityCheck even if it is the first time the selector has run. This wouldn't be too bad, however this first call is called with empty objects instead of the actual result of the selector. Please see the example below. Obviously this is a bit of a contrived example using strings, but things get pretty hairy if you expect the result to be a Set or another complex object.

const firstMessageSelector = createSelector(
  [(messages) => messages[0]],
  (message) => `${message}!!!`,
  {
    memoizeOptions: {
      resultEqualityCheck: (a, b) => {
        console.log("resultEqualityCheck:", { a, b });
        return a === b;
      },
    },
  }
);

const one = firstMessageSelector(["one", "two"]);
console.log("one:", one);
const two = firstMessageSelector(["two", "three"]);
console.log("two:", two);

Expected Console Output:

firstMessageSelector: one
one: one!!!
firstMessageSelector: two 
resultEqualityCheck: Object { a: "one!!!", b: "two!!!" }
two: two!!! 

Actual Console Output:

firstMessageSelector: one
resultEqualityCheck: Object { a: {}, b: {} }
one: one!!!
firstMessageSelector: two 
resultEqualityCheck: Object { a: "one!!!", b: "two!!!" }
two: two!!! 

keegan-lillo avatar Jan 31 '24 06:01 keegan-lillo

hmm, this is a good point - ~~the resultEqualityCheck is typed to only expect the result of the memoized function~~ (edit: it's actually typed as any), but the inputStabilityCheck uses the same memoizeOptions and memoize with a different function, one that always returns a new empty object. This is important for how the check works.

I'm not sure of the best fix for this, but I agree it's not great.

EskiMojo14 avatar Jan 31 '24 13:01 EskiMojo14

Really excited about reselect v5 but +1 on this being upgrade blocking 😞 . We almost guarantee a (development-only) runtime break when resultEqualityCheck does any sort of complex work. (and disabling inputStabilityCheck feels like a band-aid fix)


I could be missing something @EskiMojo14, but this is the code I would have expected for inputStabilityCheck:

const { inputSelectorResults, inputSelectorResultsCopy } =
    inputSelectorResultsObject
const areInputSelectorResultsEqual =
   (memoizeOptions?.equalityCheck ?? shallowEqual)(inputSelectorResults, inputSelectorResultsCopy)

I don't think resultEqualityCheck or maxSize should matter/not sure why we need to setup the full memoize again?


If this ^ doesn't work, maybe the best option is just omitting resultEqualityCheck from the ...memoizeOptions spread.

braeden avatar Feb 11 '24 00:02 braeden

we need to treat memoizeOptions as an opaque value, because it could be anything depending on what memoizer is being used. For example, weakMapMemoize's options don't have an equalityCheck property.

The only way (I can think of) that we can accurately assess that the memoizer, when configured the same way as it is for the output selector, considers the inputs to be different is to do what we do - memoize a separate function that always returns a new reference, with the exact same options used when memoizing the output selector.

EskiMojo14 avatar Feb 11 '24 20:02 EskiMojo14