Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(perf): document the correct way reference to state.firestore when creating selectors #614

Closed
jancama2 opened this issue Jan 10, 2019 · 5 comments
Milestone

Comments

@jancama2
Copy link

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
For some reason i dont understand the reference to state.firestore is changed with every action.
When using default memoization used by reselect's createSelector, the data is never memoized (the default memoization uses reference equality). This issue causes too many unnecessary updates of any react component that depends on the data from state.firestore.

import { createSelector } from 'reselect';

const selectFirestore = state.firestore;
const selectData = createSelector(
     selectFirestore,
     firestore => firestore.data,
)

// The reference of the data is changing with every action of redux-firestore
const data = selectDate(state);

// This causes any react component using 'data' to re-render even if it is unnecessary
<Component data={data} />

What is the expected behavior?
The reference to the main object shouldn't be changing with every actions so the default memoization using object reference would work.

My hack is to memoize state.firestore using lodash's isEqual. This solution is not optimal due to the complexity of the comparison.

import { createSelectorCreator, defaultMemoize } from 'reselect';
import { isEqual } from 'lodash';

export const selectFirestore = createSelectorCreator(defaultMemoize, isEqual)(
    state => state.firestore,
    firestore => firestore,
);
@prescottprue
Copy link
Owner

Thanks for reporting! Performance has been a big focus for the v1.0.0 redux-firestore buildout that is currently going on (installable from the alpha tag). What version are you currently running?

@jancama2
Copy link
Author

I'm using [email protected] and [email protected], i've just realized i've put the issues into the wrong project, but to be honest i had quite similar problems with the state.firebase object and fixed it the same way :)

@prescottprue prescottprue added this to the v3.0.* milestone Jan 15, 2019
@illuminist
Copy link
Contributor

illuminist commented Mar 5, 2019

I don't think memoizing state.firestore object is necessary, but instead "antipattern". createSelector is used for memoize function with expensive calculation and usually should have an relevant object as it's argument and firestore object is too shallow to be memoized as any change in firestore state will update the state reference.

To sum up about this issue, state.firestore needs to be changed every actions that update it's state. That's how redux works.

For correct example

const netTotal  = createSelector(
  state => _.get(state, 'firestore.data.products'),
  products => _.sumBy(products, 'price')
)

With this reselect will memoize products object instead, and even if there's any update to other query or redux firestore internal state, the memoized products object will stay the same until there's any actual update to the query or documents.

@prescottprue
Copy link
Owner

@illuminist I agree - we should add more to the docs about this

@prescottprue prescottprue changed the title bug(memoization): the reference to state.firestore is changed with every action docs(perf): document the correct way reference to state.firestore when creating selectors Sep 3, 2019
prescottprue pushed a commit that referenced this issue Sep 3, 2019
* feat(auth): remove signIn option from createUser (new user is automatically signed in through Firebase SDK) - #513
* docs(perf): document the correct way reference to state.firestore when creating selectors - #614
* chore(deps): update lodash to 4.17.15
@prescottprue prescottprue mentioned this issue Sep 5, 2019
3 tasks
prescottprue pushed a commit that referenced this issue Sep 5, 2019
@prescottprue prescottprue mentioned this issue Sep 5, 2019
3 tasks
prescottprue added a commit that referenced this issue Sep 5, 2019
* chore(deps): update to lodash to 4.17.15
* chore(deps): update proptypes to 15.7.2
* docs(integrations): add docs for how to reference data from state for reselect selectors - #614
@prescottprue
Copy link
Owner

Closing since a reselect section with a similar example has been added to both latest and next.

Thanks to everyone for posting

prescottprue added a commit that referenced this issue Sep 6, 2019
* feat(auth): remove `signIn` option from createUser (new user is automatically signed in through Firebase SDK) - #513
* feat(core): new pattern for getting extended firebase instance in thunks (added back `getFirebase` to api) - #635
* fix(HOCs): switch to `UNSAFE_componentWillReceiveProps` in class based HOCs to prevent warnings with 16.9.0 - #755
* fix(HOCs): switch `withFirebase` and `withFirestore` back to pre-hooks compatible logic
* fix(core): replace lodash methods such as `isArray`, `isBoolean`, `isString`, `size`, `compact` and `isFunction` with native methods in a number of places
* chore(deps): update lodash to 4.17.15
* chore(docs): add docs for how to reference data from state for reselect selectors - #614
* chore(docs): update client side role assign example in roles recipes - #699
* chore(docs): add example for assigning role in cloud function - #699
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants