Regression when using lodash for an unbounded cache and reselect 4.0

[nicolaserny]

I have the following unit test inspired from the reselect documentation (Use memoize function from lodash for an unbounded cache section)

import { createSelectorCreator } from "reselect";
import memoize from "lodash.memoize";

// generic hash function provided by reselect to create custom selector
const hashFn = (...args) =>
  args.reduce((acc, val) => acc + "-" + JSON.stringify(val), "");

const unlimitedCacheSelectorCreator = createSelectorCreator(memoize, hashFn);

describe("unlimitedCacheSelectorCreator", () => {
  const selectorCreatorTest = [
    { id: 0, expected: ["test0"], recomputations: 1 },
    { id: 2, expected: ["test2"], recomputations: 2 },
    { id: 0, expected: ["test0"], recomputations: 2 },
    { id: 1, expected: ["test1"], recomputations: 3 },
    { id: 0, expected: ["test0"], recomputations: 3 },
    { id: 2, expected: ["test2"], recomputations: 3 },
    { id: 1, expected: ["test1"], recomputations: 3 }
  ];

  const getId = (_, id) => id;
  const stateExample = [
    { id: 0, name: "test0" },
    { id: 1, name: "test1" },
    { id: 2, name: "test2" }
  ];

  const selector = unlimitedCacheSelectorCreator(
    [state => state, getId],
    (stateValue, id) =>
      stateValue.filter(val => val.id === id).map(val => val.name)
  );

  selectorCreatorTest.forEach(test => {
    it("Selector should get values using an unlimited cache", () => {
      expect(selector(stateExample, test.id)).toEqual(test.expected);

      expect(selector.recomputations()).toEqual(test.recomputations);
    });
  });
});

All the tests passed with reselect 3.0.1.
After upgrading to reselect 4.0, 6 tests keep on failing.
It seems that the hash function is called once. Therefore the selector always returns the same value (instead of the right value).
Is there anything wrong with my code ?

[nicolaserny]

A quick look at the code shows that my problem is related to this change.

[mcrawshaw]

I can confirm the same issue here. Tested with both versions. Your pull @mosho1.

[srolel]

From lodash docs:

By default, the first argument provided to the memoized function is used as the map cache key.

In the test case, the first argument is the state object, and all calls return the same memoized value after the first call as expected. This should make the test cases pass by having memoization done with the ids as the cache keys:

const unlimitedCacheSelectorCreator = createSelectorCreator(fn => memoize(fn, getId), hashFn);

[nicolaserny]

In this solution, we mix the map cache key definition and the memoization function.

The purpose of the hashFn function is to compute the map cache key.
From reselect documentation:

const customSelectorCreator = createSelectorCreator(
  customMemoize, // function to be used to memoize resultFunc
  option1, // option1 will be passed as second argument to customMemoize
  option2, // option2 will be passed as third argument to customMemoize
  option3 // option3 will be passed as fourth argument to customMemoize
)

Therefore, memoize should use my hashFn function to get the map cache key.

[srolel]

Yes, you are right. This PR makes the required change: #359

[markerikson]

Went out in https://github.com/reduxjs/reselect/releases/tag/v4.0.0 .

[mcrawshaw]

@markerikson What do you mean? The PR was released in that tag? I believe the issue was raised due to that tag.

[markerikson]

You're right, my bad. I was looking at the wrong commit diff and got confused, sorry!

[markerikson]

I just tried making this change on top of master, with the latest change from #513 . It immediately made this other test start failing:

  test('custom memoize', () => {
    const hashFn = (...args: any[]) =>
      args.reduce((acc, val) => acc + '-' + JSON.stringify(val))
    const customSelectorCreator = createSelectorCreator(lodashMemoize, hashFn)
    const selector = customSelectorCreator(
      (state: StateAB) => state.a,
      (state: StateAB) => state.b,
      (a, b) => a + b
    )
    
    // snip
  })

with this error:

    TypeError: Expected a function

      118 |     // If a selector is called with the exact same arguments we don't need to traverse our dependencies again.
      119 |     // @ts-ignore
    > 120 |     const selector: OutputSelector<any, any, any, any> = memoize(function () {
          |                                                          ^
      121 |       const params = []
      122 |       const length = dependencies.length
      123 |

      at memoize (node_modules/lodash/memoize.js:52:11)
      at createSelector (src/index.ts:120:58)
      at Object.<anonymous> (test/test_selector.ts:304:22)

I think the issue may be specifically related to use of Lodash here, but I'm not sure.

Given that, I'm not comfortable trying to make this change right now. If someone else would like to take a look and figure out what's going on, please do so.

[dobernike]

the same issue with lodash/fp: option1 is not a function:
a little logs in createSelectorCreator(ourCustomMemoize, flip(difference)):

option1 [Function (anonymous)]
option1 undefined
option1 [Function: l]
option1 undefined

const ourCustomMemoize = (func, option1) => {
    let prev;
    let initialised = false;
    return curr => {
        if (curr !== prev) {
            const res = initialised ? func(option1(prev, curr)) : false;
            prev = curr;
            initialised = true;
            return res;
        } else {
            return false;
        }
    };
};

works in reselect 2 and 3, but not in 4

[markerikson]

FWIW, this issue is extremely low priority atm. If someone wants to look at what's going on here that would be great!

[aryaemami59]

I believe this has been reoslved by #626 due to the implementation of argsMemoize.