The Wayback Machine - https://web.archive.org/web/20200916141456/https://github.com/reduxjs/reselect/pull/321
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

Allow selector dependencies to be selector factories #321

Open
wants to merge 4 commits into
base: master
from

Conversation

@madcapnmckay
Copy link

madcapnmckay commented Feb 9, 2018

As noted in the docs, if you are not careful, memoization can be undone when using a selector with props in multiple components.

The solution is to use a selector factory and pass that to mapStateToProps instead. This works well for one level deep selectors, however, if we are composing memoized selectors the same caveats apply. It is very easy to unintentionally undo the memoization as we nest. The solution again is to keep wrapping our sub-selectors in factories and executing them as we create the selectors.

A neater approach is to always accept either a selector or a selector factory in the createSelector function and handle it in a similar manner as react-redux connect does. This would allow us nest selector factories without having to explicitly execute the factory.

This change means the way selectors are used is identical whether they are

  • Pure Functions
  • Memoized Singletons
  • Reusable Memoized Factories
  • Composed from the above types
  • Used directly in mapStateToProps

For example:

// moduleA.js
export somePureSelector = (state, props) => state.a + props.b;

// moduleB.js
export someMemoizedSelector = () => createSelector(
  (state) => state.a,
  (state, props) => props.b,
  (a, b) => a + b
);

// moduleC.js
export composedSelector = () => createStructuredSelector({
   a: somePureSelector,
   b: someMemoizedSelector
});

// ContainerA.js
export connect(somePureSelector)(SomeComponent);

// ContainerB.js
export connect(someMemoizedSelector)(SomeComponent);

// ContainerC.js
export connect(composedSelector)(SomeComponent);

I realize this PR is incomplete, I just wanted to get people's opinion on the change.

@coveralls
Copy link

coveralls commented Feb 9, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7c2719a on madcapnmckay:selectorFactories into 7cab533 on reactjs:master.

@codecov-io
Copy link

codecov-io commented Feb 10, 2018

Codecov Report

Merging #321 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #321   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          15     16    +1     
=====================================
+ Hits           15     16    +1
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cab533...7c2719a. Read the comment docs.

@toomuchdesign
Copy link

toomuchdesign commented Feb 18, 2018

Hi @madcapnmckay, just to point out that re-reselect, let you declare selectors which internally behave like a factory but keep the same API of a normal reselect selector outside.

This allows forgetting differences between selectors and selector factories.

@abritinthebay
Copy link

abritinthebay commented Sep 28, 2018

@toomuchdesign while true, that's somewhat different to the proposed solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.