Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

weakMapMemoize with resultEqualityCheck is provided empty objects for first call. #693

Open
keegan-lillo opened this issue Jan 31, 2024 · 3 comments

Comments

@keegan-lillo
Copy link

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!!! 
@EskiMojo14
Copy link
Contributor

EskiMojo14 commented Jan 31, 2024

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.

@braeden
Copy link

braeden commented Feb 11, 2024

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.

@EskiMojo14
Copy link
Contributor

EskiMojo14 commented Feb 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants