Skip to content
This repository has been archived by the owner on May 6, 2019. It is now read-only.

refactor(Selectors): Use reselect for selectors instead of Rx operators #75

Merged
merged 1 commit into from
Nov 30, 2016

Conversation

MikeRyanDev
Copy link
Member

@MikeRyanDev MikeRyanDev commented Nov 23, 2016

In a real world application we've discovered that the performance cost of using Rx operators to perform joins can be significantly higher in comparison to memoized functions.

For example, consider this group of selectors:

export function getIds(state$: Observable<State>) {
  return state$.select(state => state.ids);
}

export function getEntities(state$: Observable<State>) {
  return state$.select(state => state.entities);
}

export function getAll(state$: Observable<State>) {
  return combineLatest(getIds(state$), getEntities(state$))
    .map(([ ids, entities ]) => ids.map(id => entities[id]));
}

The characteristics of the three selectors in marble diagrams will look like this:

IDs:      --a-----b--
Entities: -----c--d--
All:      --a--c--(bd)--

Importantly, on the frame where both IDs and Entities changes the combineLatest observable emits twice: once for each update. This means that the map function joining the two pieces of state will be called twice in the same tick and each subscriber will be notified twice.

This PR refactors the selectors to use reactjs/reselect for fast, memoized selectors.

The above three selectors can rewritten like this:

const getIds = (state: State) => state.ids;

const getEntities = (state: State) => state.entities;

const getAll = createSelector(getIds, getEntities, (ids, entities) => {
  return ids.map(id => entities[id]);
});

Applying selectors is done with select instead of let:

// before
store.let(getAll);

// after
store.select(getAll);

cc @btroncone @dpsynapse

@MikeRyanDev MikeRyanDev changed the title chore(Selectors): Use reselect for selectors instead of Rx operators refactor(Selectors): Use reselect for selectors instead of Rx operators Nov 23, 2016
@btroncone
Copy link
Collaborator

Other than having to include an additional library are there any downsides to this approach? I'm only seeing wins, both in performance and readability.

@brandonroberts
Copy link
Member

brandonroberts commented Nov 28, 2016

Are there not any combinations of rxjs operators that would accomplish this? Not arguing against the change, but asking for my own knowledge.

@MikeRyanDev
Copy link
Member Author

@brandonroberts Not really.

@btroncone Can't think of any major downsides, though I did think of another upside: memoization works across multiple subscriptions.

@MikeRyanDev
Copy link
Member Author

Merging this in :)

@MikeRyanDev MikeRyanDev merged commit 0bd8824 into master Nov 30, 2016
@MikeRyanDev MikeRyanDev deleted the chore/use-reselect-for-selectors branch November 30, 2016 00:45
@karptonite
Copy link

karptonite commented Dec 2, 2016

Am I correct that the reselect library is only actually being used for selectors such as getAll in the example above, not for the simple selectors such as getIds? I see the library being imported in collection.ts, for example, but not being used at all. I see that when the reducers are combined in the index.ts file, createSelector is used, but it looks like that import is unused in some of the other files?

@marklagendijk
Copy link

Would reselect be faster than the following solution?

export function getIds(state$: Observable<State>) {
  return state$.select(state => state.ids);
}

export function getEntities(state$: Observable<State>) {
  return state$.select(state => state.entities);
}

export function getAll(state$: Observable<State>) {
  return Observable
    .combineLatest(getIds(state$), getEntities(state$), (ids, entities) => ids.map(id => entities[id]))
    .distinctUntilChanged()
}

If so, you could just create an operator which combines these two operators, right? You could call it something like combineDistinctLatest.
That should be just as performant as reselect, right?

@bypotatoes
Copy link
Contributor

@marklagendijk By default distinctUntilChanged compares values with ===, so in this case it won't work, since map return new array and [] === [] // false and values in stream never be "the same". To get result that you're expecting pass callback to distinctUntilChanged, that implements custom compare logic

@mrpmorris
Copy link

When I wrote my own rxjs implementation of selectors for NGRX/Store I got around this problem by adding a debounce of 1 tick for selectors that take multiple sources.

@mfp22
Copy link

mfp22 commented Feb 11, 2018

@mrpmorris that's okay if you're okay with triggering an extra zone run.

Rather than createSelector, why not create an operator selectAll that uses distinctUntilChanged that checks each input? Then you don't have to go outside RxJS. One of the reasons I switched from Redux + Reselect is because RxJS operators are much cleaner to me than memoized selectors.

Here's an example implementation:

Rx.Observable.prototype.selectAll = function(maps) {
  return this.map(val => maps.map(map => map(val)))
    .distinctUntilChanged((ar1,ar2) => ar1.reduce(
      (all,el,i)=>all&&ar1[i]===ar2[i]
    , true));
}

Here's some example output that creates:

"selectAll 0,1"
"combineLatest 0,1"

"selectAll 1,0"
"combineLatest 1,1"
"combineLatest 1,0"

Here's the demo: https://goo.gl/G9uPT5

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants