createSelector() not memoizing correctly

[03byron]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request

What is the current behavior?

The projector fn passed to createSelector() is invoked even when its arguments remain unchanged.

Expected behavior:

The projector fn should be called only when one of its arguments change.

Minimal reproduction of the problem with instructions:

http://plnkr.co/edit/GEdUHwk3XEnHHwmPVMSr?p=preview

When you click increase/decrease buttons you can see in the console that the projector function executes but its arguments didnt change.

or in short summary:

const mySelector = createSelector(
  (state) => state.substate,
  (substate) => {
    // heavy computing here, should be run only when substate changes,
    // but is running whenever mySelector is called.
    return substate.something;
  }
);

interestingly when wrapping the projector fn in createSelector as well, i get the desired behavior:

const mySelector = createSelector(
  (state) => state.substate,
  createSelector( s => s, (substate) => {
    // heavy computing here, is called only when substate changes
    return substate.something;
  })
);

Version of affected browser(s),operating system(s), npm, node and ngrx:

Chrome 59
OS X El Capitan
npm 3.10.8
node 6.9.1
ngrx 4.0.0

[karptonite]

OK, I can confirm that this is an issue.
If we substitute createSelector from reselect, the bug goes away.
http://plnkr.co/edit/ygjOlw9I2WOqqMAo83Wf?p=preview

I understand the desire not to have ngrx dependent on other packages, but these sorts of bugs could be avoided by not trying to reinvent things like createSelector or even memoize which are well implemented in other libraries.

[karptonite]

One other data point anecdote: I ran into a similar issue the other day that I didn't report because it was difficult to reproduce. Here is how it manifested:
I had a similar issue where an Observable from a selector was emitting values even though the inputs had not changed. However, the bug only appeared when I was 1) serving via ng serve in dev mode and 2) had just done a hard reload in the browser. If the page was reloaded by angular cli with hot module reload, the bug went away. Very odd, and difficult enough to track down/reproduce that I didn't take the time to write it up. but it wouldn't surprise me if it were related.

If you have an example of this bug in your app, can you try building it with --prod (if you are using angular cli), and see if the problem goes away? It did for me.

[03byron]

Just did a ng build --prod and the issue still exists.

[03byron]

I've created a failing test. How am i able to submit it?

Actually, one could argue that some tests are wrong:

For example:

    it('should memoize the function', () => {
      const firstState = { first: 'state' };
      const secondState = { second: 'state' };
      const projectFn = jasmine.createSpy('projectionFn');
      const selector = createSelector(
        incrementOne,
        incrementTwo,
        incrementThree,
        projectFn
      );

      selector(firstState);
      selector(firstState);
      selector(firstState);
      selector(secondState);

      expect(incrementOne).toHaveBeenCalledTimes(2);
      expect(incrementTwo).toHaveBeenCalledTimes(2);
      expect(incrementThree).toHaveBeenCalledTimes(2);
      expect(projectFn).toHaveBeenCalledTimes(2);
    });

projectFn should have been called 4 times in this example, because all selectors of the projectFn return a new value each time they are called:

  beforeEach(() => {
    countOne = 0;
    countTwo = 0;
    countThree = 0;

    incrementOne = jasmine.createSpy('incrementOne').and.callFake(() => {
      return ++countOne;
    });

    incrementTwo = jasmine.createSpy('incrementTwo').and.callFake(() => {
      return ++countTwo;
    });

    incrementThree = jasmine.createSpy('incrementThree').and.callFake(() => {
      return ++countThree;
    });
  });

https://github.com/ngrx/platform/blob/master/modules/store/spec/selector.spec.ts#L44

[03byron]

I think the source of the issue is that memoization is based on the (whole) state and not on the value of the selectors.

https://github.com/ngrx/platform/blob/master/modules/store/spec/selector.spec.ts#L44

[karptonite]

projectFn should have been called 4 times in this example, because all selectors of the projectFn return a new value each time they are called:

I think the source of the issue is that memoization is based on the (whole) state and not on the value of the selectors.

I think that you are mistaken in your first point. projectFn should only be called 2 times, precisely because the memoization is based on the whole state. And I believe that is the correct behavior--if the whole state is identical, the selector should be able to skip ALL the selectors and the projector and just return the same result as last time.

The problem is that it should probably also be checking all the results of the selectors, and skipping the projector if those are also identical. It doesn't appear to be doing that. I haven't dug into the reselect code, but my guess is that reselect does this.