RFC: feat(selectors): SelectorFactoryWithCache - allow to re-use selectors for props

[nadavsinai]

Memoized Selectors are a powerful way to wrap computed state logic in a simple yet performant functional casing. We love them.
Selectors with props solve the problem of computing (perhaps expensive) state that relies on runtime supplied values, Howvever when put to test one finds (and can be confused by) the fact that props are checked for referential equality, if the prop type is not scalar (eg object) each new value thrown at the selector will cause all the projection functions to be re-invoked.

Even team members advise to use object props:
#1233 (comment)
which will cause further invocations to loose memoization

And community issues like this arise
#1415

However sometimes the set of runtime values (props) relevant to a computed state is limited, caching selectors for each prop value allows us to keep the memorization and work done in the past.

For example in our medical imaging app we have a grid of canvases, each one has its own state model which is computed by merging few other states. we have a selector that needs the prop of layoutID (identifying that canvas) which is a runtime value, but has limited range. When a user switches layouts or view modes a lot of scripting is happening, we creating the concept of CachedSelectorFactories to reduce some of that scripting time.

The use of the selector from the component/service side is same as selectors without props
this.store.select(selectImageboxModelByLayoutID(layoutID))

but the selector is created by calling this factory function is cached.

export const selectImageboxModelByLayoutID = createSelectorFactoryWithCache((layoutID: string) =>
	createSelector(
		selectFirstModelForImageboxModelLayoutID(layoutID),
        ...furtherSelectorsByLayoutID(layoutID),
		(...projectedModels) => {
			return {...projectedModels}; // further expensive projection code here...
		}
	)
);

the signature of that function is this

function createSelectorFactoryWithCache<State, Props, Result>(
  selectorFactory: SelectorFactoryByParams<State,Props,Result>
): SelectorFactoryWithParam<State, Props, Result>

whilst that types are:

interface SelectorFactoryByParams<State,Props,Result> {
  (props: Props): MemoizedSelector<State, Result>;
}
interface SelectorFactoryWithParam<State, Props, Result> extends SelectorFactoryByParams<State,Props,Result>{
  release(): void;

  setResult(value: any): void;
}

SelectorFactoryByParams is a factory function creating a selector in the closure of the param.
we only invoke this function if we don't have it cached. I used a Map -> Map<Props, MemoizedSelector<State, Result>> to keep selectors and the release function allows to clear memory when the expected props are not relevant anymore.

The full code is available here , I will open a PR if/when I get positive input on this.

Describe any alternatives/workarounds you're currently using

We have this functionality in an internal lib. It works well for us and allowed us to improve application performance.

If accepted, I would be willing to submit a PR for this feature

[X] Yes (Assistance is provided if you need help submitting a pull request)

Will need guidance with regards to tests you'd like to add and perhaps improve typing.

[nadavsinai]

@itayod I finally got to write the issue RFC I told you about in Angular-UP...I Would be happy for your input.

[timdeschryver]

I'm interested in the use case described.
Is the caching needed because you're using the selector at multiple places at the same time in your app (for example in componentOne and componentTwo)?
Or do you want to have a cache if a user goes back to the previous result (layout 1, layout 2, layout 1 loaded from cache)?

[itayod]

@timdeschryver If I got it right, the use case here is for caching selectors with params, similar to the memoized selectors and save computation (e.g when the user toggles between layout).
@nadavsinai IMHO it does make sense :) just for curiosity how long does this kind of computation could last in your application?

[nadavsinai]

yes, the selector factory created is used in multiple places in the application - components, services (effects etc) - since it caches the results of calls with params it can eliminate the work when called again with same param.
it is very simply shown in the following test:

 		  const times5 = selectCMultipliedBy(5)(mockState);
          const times7 = selectCMultipliedBy(7)(mockState);
          const times5again = selectCMultipliedBy(5)(mockState);
          expect(insideProjection).toHaveBeenCalledTimes(2);

[nadavsinai]

should I open a PR?

[REPLicated]

@nadavsinai : If not just the single result for the last supplied state and props were to be cached, but the results for all different props ever evaluated given that same state: How do you intend to prevent unbounded memory consumption? You would need some kind of cache management.

By the way, what you suggest sounds pretty much like https://github.com/toomuchdesign/re-reselect in the redux/react ecosystem (whereas the selectors in ngrx seem to be designed like https://github.com/reduxjs/reselect).

[timdeschryver]

If you want to use the selector at different locations you can use a selector with props. The only difference with your implementation if that it loses its cache if the entityID is changed.

https://gist.github.com/timdeschryver/43a3337e09a0e1e857d11c317d13439c

[nadavsinai]

Hi, thanks for your comments.
@timdeschryver I know that the normal SelectorWithProps passes all but that failing test which is exactly the one I pointed out to show the difference in behavior.
@itayod in our application the cached selectors made a difference : when switching layout it was a change that eliminated 2-3 seconds of scripting.
@REPLicated I was not aware of re-reselect, it solved the exact same problem (in the same way), I actually take this as re-assurance that this is needed for the wider community and not only in our special use case. now the thing is, one can argue we should use the community's already published code and direct users to use reselect and re-reselect with store, but since the store package already has its own implementation of createSelector i'd say this needs to be included too.
About cache control -I added a way to flush it, but we can certainly learn from what re-reselect does and if you wish I can add a cacheControl object to let the user decide which selector to keep and general finer grain control of the cache

[nadavsinai]

I'm interested in the use case described.
Is the caching needed because you're using the selector at multiple places at the same time in your app (for example in componentOne and componentTwo)?
Or do you want to have a cache if a user goes back to the previous result (layout 1, layout 2, layout 1 loaded from cache)?

both are right. selectors are defined in thier own file and used across the application, but we use createSelectorFactoryWithCache specifically in places where the props are changing (yet in a bounded set of values - eg layout modes)

[timdeschryver]

Got it, thanks for elaborating @nadavsinai 👍

[nadavsinai]

HI, @itayod @timdeschryver @REPLicated
I've done some work on that idea, I've added a new API which works with
store.select(selector,props) API and also added cache options.
I’ll be happy to hear your input, I think about ditching the original API I proposed for the sake of the simpler newer one, let me know if you see any downsides.
Also I want to hear if you have any ideas how to make this better typed - hopefully without repeating all the overloads for createSelector)

[timdeschryver]

Thanks for putting in the time by creating a PR.

I would suggest to wait now before we take the next steps to make sure this is a good addition to add in @ngrx/store.

[brandonroberts]

@nadavsinai thanks for your work on this. I think its an interesting idea, but I also don't know if there is enough value to adding another API for creating selectors. Why wouldn't this just be a guide on creating a cache for selectors with props? We haven't seen enough people with this issue to consider adding this to the API yet.

[jpduckwo]

When using selectors with object props I'm getting situations where the selector emits multiple times over with actions that don't even modify the selectors states.

Everything seems to go back to normal when I put the selector into a factory function as recommended in the docs to improve memoization with selectors that are used with different props.

However what I'm seeing is something unexpected even without the factory function. I'm unable to replicate it in a stackblitz as yet... but commenting here in case anyone else has seen something similar. I feel it could be something linked to this issue.

https://ngrx.io/guide/store/selectors

Keep in mind that a selector only keeps the previous input arguments in its cache. If you re-use this selector with another multiply factor, the selector would always have to re-evaluate its value. This is because it's receiving both of the multiply factors (e.g. one time 2, the other time 4). In order to correctly memoize the selector, wrap the selector inside a factory function to create different instances of the selector.

[brandonroberts]

Going to close this along with the PR for now. We may revisit it at some point.