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

Pass all args to the selector function #95

Closed
wants to merge 1 commit into from
Closed

Pass all args to the selector function #95

wants to merge 1 commit into from

Conversation

mdebbar
Copy link

@mdebbar mdebbar commented Mar 15, 2016

Why do we have to special-case props? Let's pass all the arguments to the dependencies and to the result function. I'm still new to Reselect so I'm curious to know if I'm doing something wrong here.

This change will allow us to create selectors that take arguments:

const allBooks = (state) => state.books;

const oldBooks = createSelector(
  [allBooks],
  (books, yearLimit) => books.filter(book => book.publishingYear < yearLimit)
);

oldBooks(state, 1990); // recomputation (initial computation)
oldBooks(state, 1990); // from cache

const booksByAuthorId = createSelector(
  [allBooks],
  (books, authorId) => books.find(book => book.authorId === authorId)
);

booksByAuthorId(state, 123); // recomputation (initial computation)
booksByAuthorId(state, 123); // from cache

It can be used with react-redux as follows:

const mapStateToProps = (state, ownProps) => ({
  oldBooks: oldBooks(state, ownProps.yearLimit),
  booksByAuthorId: booksByAuthorId(state, ownProps.authorId),
});

const MyConnectedComponent = connect(mapStateToProps)(MyComponent);

See my comments for more context.

@ellbee
Copy link
Collaborator

ellbee commented Mar 18, 2016

Props looks special cased because it won't work with React Redux otherwise. See #40 and #68. I'll add a comment to the code about this because it is highly non-obvious.

The reasoning as to why we don't currently support selectors that take arguments can be found here.

@ellbee ellbee closed this Mar 18, 2016
@mdebbar
Copy link
Author

mdebbar commented Mar 18, 2016

If I'm understanding the suggestions correctly, there is a big issue that they introduce:

const allBooks = (state) => state.books;
const oldBooks = createSelector(
  allBooks,
  (books) => memoize(
    yearLimit => books.filter(book => book.publishingYear < yearLimit)
  )
);

const getOldBooks = oldBooks(state);

getOldBooks(1990); // works fine.

// State gets updated...

getOldBooks(1990); // returns out-dated results :(

The problem is that memoized function that I created is recording an old state and never getting the new state. Unaware users will definitely fall for this and have nasty bugs.

I've been reading the issues and discussions about dynamic arguments but I still don't understand the reason for not supporting them. I understand that there are workarounds for some cases (e.g. putting the dynamic argument in the state). But that doesn't solve all the use cases.

I guess what I'm looking for is: what's exactly wrong with this PR? Does it introduce any issues that I'm not aware of? Does it add a lot of concepts that make the library difficult to learn/use?

If you read through my comments in the closed issue, I've explained why all the workarounds don't work in my case.

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

Successfully merging this pull request may close these issues.

2 participants