Search code examples
javascripttypescriptngrx

NGRX selectors updating too often


I am really confused, I really thought that selectors would not run if all of his parents returned the same result.

At a time, there is 1-250 clusterMarker selector active, each with a different prop, cluster. Its execution is rather expensive. I made sure, that it needs to be reevaluated only if the result of any of its arguments changes.

Simplified example:

const offerState = createFeatureSelector<OfferState>('offer');
const currentCarrier = createSelector(offerState, state => state.currentCarrier);
const currentContext = createSelector(offerState, state => state.currentContext);
const currentPeriod = createSelector(offerState, state => state.currentPeriod);
const filteredCarriers = createSelector(offerState, state => state.filteredCarriers);
const wanted = createSelector(offerState, state => state.wanted);

const filteredCarriersSet = createSelector(
    filteredCarriers,
    carriers => new Set(carriers),
);

/**
 * Only fire if the changes to state affect this context.
 * `undefined` => `state.currentCarrier`
 * `state.currentCarrier` => `undefined`
 */
const currentCarrierInCluster = createSelector(
    currentCarrier,
    currentContext,
    (
        currentCarrier: Carrier | null,
        currentContext: AbstractMapClusterContext<Carrier> | null,
        { cluster }: { cluster: AbstractMapClusterContext<Carrier> }
    ) => currentContext == cluster ? currentCarrier : undefined,
);

export const clusterMarker = createSelector(
    filteredCarriersSet,
    currentCarrierInCluster,
    currentPeriod,
    wanted,
    (
        filteredSet,
        currentCarrier,
        currentPeriod,
        wanted,
        { cluster }: { cluster: AbstractMapClusterContext<Carrier> }
    ) => {
        // ... code ...
    },
);

Is there a part of the documentation about setting memorization options I missed? What can I do, to make this more performant?

Response to answer:

Code:

export const clusterMarkerSelectorFactory = () =>  createSelector(
    filteredCarriersSet,
    currentCarrierInCluster,
    currentPeriod,
    wanted,
    (
        filteredSet,
        currentCarrier,
        currentPeriod,
        wanted,
        { cluster }: { cluster: AbstractMapClusterContext<Carrier> }
    ) => {
        // ... code ...
    },
);

class Component {
    constructor(
        private store$: Store<OfferState>,
    ) { }

    readonly state$ = this.cluster$.pipe(
        switchMap(cluster => this.store$.select(clusterMarkerSelectorFactory(), { cluster })),
    );
}

This will still retrigger for every one of them.


Solution

  • I now know why this happens, I have also had several workarounds. It all is tied to the fact that whenever a selector is called with parameters, all of its parents will be called with parameters too! I have created a GitHub issue on this.

    tldr;

    Original:

    // replace
    const elementSelectorFactory = () => createSelector(
        listAsSet,
        (set, { element }) => set.has(element),
    );
    

    Fixed v1:

    // with (This needs to be done only for the first level of selectors, more info bellow)
    const elementSelectorFactory = () => createSelector(
        state => listAsSet(state as any),
        (set, { element }) => set.has(element),
    );
    

    Fixed v2:

    // or with
    const elementSelectorFactory = element => createSelector(
        listAsSet,
        set => set.has(element),
    );
    
    // or in case you have nested props selectors (my case)
    const elementSelectorFactory = element => createSelector(
        listAsSet,
        nestedSelectorFactory(element),
        (set, nested) => set.has(element) ? nested : null,
    );
    

    I will walk you through a simple selector with props.

    We have code bellow

    interface Store {
      elements: unknown[];
    }
    
    export const offerState = createFeatureSelector<Store>('store');
    
    const listAsSet = createSelector(
        state => state.list,
        carriers => new Set(carriers),
    );
    
    // We create a factory like they instructed us in the docs
    const elementSelectorFactory = () => createSelector(
        listAsSet,
        (set, { element }) => set.has(element),
    );
    

    Now if we call this.store$.select(elementSelectorFactory(), { element }).

    1. We create the elementSelector
    2. This selector will ask for data from its parent with the following parameters: [state, { element }].
    3. listAsSet gets called with state and props { element }, even though it does not use the props.

    Now, if you were to create 2 elementSelector from the factories, step 3 will be called twice, with 2 different props! Since its arguments are not equal it must be recalculated. Set() of the same elements are not equal, thus this will invalidate arguments of our elementSelector!

    What is worse, if you are using the listAsSet selector somewhere else without props, it would get invalidated too!

    This was my first solution to this:

    const elementSelectorFactory = () => createSelector(
        // Here we don't pass the props to the child selector, to prevent invalidating its cache
        (state, _props) => listAsSet(state as any),
        (set, { element }) => set.has(element),
    );
    

    The next solution is not to use props at all, as suggested by ngrx member...

    // or with
    const elementSelectorFactory = element => createSelector(
        listAsSet,
        set => set.has(element),
    );
    

    Here, the selector will never receive props, to begin with... If you want to have nested selector you will have to rewrite them all to selector factories with parameter(s) to combat this.

    const elementSelectorFactory = element => createSelector(
        listAsSet,
        nestedSelectorFactory(element),
        (set, nested) => set.has(element) ? nested : null,
    );