Change what defaultMemoize compares with isEqual

[alex-okrushko]

Also renamed isEqual to isResultEqual to be clear what is compared
This is the fix for #1157

[coveralls]

Coverage increased (+0.05%) to 88.218% when pulling 45c9b8d on alex-okrushko:memoizer into 03db76f on ngrx:master.

[alex-okrushko]

@brandonroberts
To be 100% compatible with currentVersion, instead of replacing isEqual with isResultEqual I can just add it as a 3-rd argument, but I think it's not needed, because isResultEqual will be catching results even before they are passed into other sub-selectors.
Let me know what you think.

[timdeschryver]

I'm going to nitpick a bit and say that I find the name defaultMemoize a bit confusing, I would rather go for just memoize. This wouldn't mean that defaultMemoize would disappear tho.

Let me show you want I'm trying to say:

function memoize(projectionFn: AnyFn, isResultEqual: (a: any, b: any) => boolean) {
  // ...
}

function defaultMemoize(projectionFn: AnyFn) {
  return memoize(projectionFn, isEqualCheck);
}

The defaultMemoize will be used as the default in NgRx, as the name says.
But if you rather want to use your own, you can use memoize instead of defaultMemoize.

If this makes sense... 😅

[alex-okrushko]

I would rather go for just memoize

memoize is already a function inside defaultMemoize. If anything I would've changed the name to defaultMemoizer with r at the end.

The defaultMemoize will be used as the default in NgRx, as the name says.
But if you rather want to use your own, you can use memoize instead of defaultMemoize.

I'd rather not duplicate the code with the second memoize there as well.
First of all, there are very similar. The only difference is applied the results comparison.
Secondly, that would unnecessary increase the size of ngrx :P

createSelectorFactory is used within Google in a number of places. I'm patching this PR and running all the tests globally on entire codebase. Let's see if it breaks anything. (I'll report back).

[timdeschryver]

I'm not sure if I'm following, how would this increase the size?
I'm not suggesting to duplicate the code, but to call the memoize function from inside defaultMemoize.

[alex-okrushko]

So it failed a few tests that were testing memoization with custom equality
Ok, I'll adjust so it doesn't have any breaking changes

[dummdidumm]

I think adding the isResultEqual as a new third argument would be better. No breaking changes + maybe someone really wants to compare arguments and not results.

[alex-okrushko]

I've extracted result checking into its own argument, so now it's 100% backwards compatible.

PTAL :)

[brandonroberts]

@alex-okrushko I will take a look. I still think this would be a change in behavior. Selectors that fire a second time before would not if the results were the same, correct? Have you seen any impact on large sets of data?

[alex-okrushko]

@brandonroberts I don't think there would be a change in behavior. All previously written tests are exactly the same now. If 3-rd argument isResultEqual is not passed to defaultMemoize, it behaves exactly as before.

I'm reruning all the tests within G again.

Have you seen any impact on large sets of data?
If isResultEqual is used? It helps not to re-render filtered lists.

[alex-okrushko]

FYI, all Google tests pass with this change.

[alex-okrushko]

Thanks!