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

Trying to give FAQ answer I was looking for #59

Closed
wants to merge 2 commits into from

Conversation

Sigfried
Copy link

I think my answer doesn't quite work -- it won't memoize the result, right? Maybe someone can show how it could.

This is surely obvious to most people, but being new to all this, I couldn't figure it out for a long time. Maybe this will help someone else.
@ellbee
Copy link
Collaborator

ellbee commented Oct 28, 2015

HI @Sigfried, thanks for taking the time to make this PR. expensiveItemSelector returns a function, is that intentional?

There are a couple of issues (#18 and #32) that address this topic. Maybe there is some information contained in them that we could add to the FAQ answer to make it clearer. What do you think?

@Sigfried
Copy link
Author

I'm not quite understanding those.

Yeah, it's intentional, if somewhat cheesy that it returns a function. The
selector is still serving up all the items, but you get a function that
filters those when you call it. Which means memoizing is pointless.

On Wed, Oct 28, 2015 at 10:01 AM, Lee Bannard [email protected]
wrote:

HI @Sigfried https://github.com/Sigfried, thanks for taking the time to
make this PR. expensiveItemSelector returns a function, is that
intentional?

There are a couple of issues (#18
#18 and #32
#32) that address this topic.
Maybe there is some information contained in them that we could to the FAQ
answer to make it clearer. What do you think?


Reply to this email directly or view it on GitHub
#59 (comment).

@ellbee
Copy link
Collaborator

ellbee commented Oct 28, 2015

Ah, ok. So it would be used like this?

import { createSelector } from 'reselect'

const state = {
  items: [
    {name: 'thing1', value: 100},
    {name: 'thing2', value: 200},
    {name: 'thing3', value: 300},
    {name: 'thing4', value: 400},
    {name: 'thing5', value: 500}
  ]
}

const shopItemsSelector = state => state.items

const expensiveItemSelector = createSelector(
  shopItemsSelector,
  items => minValue => items.filter(item => item.value > minValue)
)

const getExpensiveForMe = createSelector(
  expensiveItemSelector,
  filterFn => filterFn(200)
)

const getExpensiveForMovieStar = createSelector(
  expensiveItemSelector,
  filterFn => filterFn(1000000)
)

console.log(getExpensiveForMe(state))
console.log(getExpensiveForMovieStar(state))

@Sigfried
Copy link
Author

Sort of. Except I imagine the selectors all defined in one place and then being used elsewhere. So out in a component somewhere that's connected to your selectors...

import * as Selectors from '../selectors';
function mapStateToProps(state) {
  return {
    expensiveItemSelector: Selectors.expensiveItemSelector(state),
  };
}
export default connect(mapStateToProps)(MyComponent);

class MyComponent extends Component {
  render() {
    return <p>Can you afford these? {this.props.expensiveItemSelector(200)}</p>
  }
}

@ellbee
Copy link
Collaborator

ellbee commented Oct 28, 2015

Right, gotcha. I guess the problem with this is, as you point out, that the function returned from expensiveItemSelector isn't memoized. I question whether using Reselect for the filtering operation is useful here—I think a plain function in the component that takes props.items as a parameter would do the job just as nicely.

@ellbee
Copy link
Collaborator

ellbee commented Oct 28, 2015

Oh, I just noticed that I linked to a wrong issue before. This is the one I meant #38.

@Sigfried
Copy link
Author

Right. What I ended up being able to (just in this case), was make a selector that returns a map of all the possible parameters I could want. Because the work is more expensive than filtering and I did want the memoizer.

So, don't accept my FAQ change; it's not helpful. But is it possible to do make a selector that can be used the way I was trying to use mine but that does actually memoize results?

@ellbee
Copy link
Collaborator

ellbee commented Oct 28, 2015

I think your solution with the map sounds like a good approach. Reselect doesn't really encourage parameterised selectors. The general consensus here and over at nuclear-js is that if a selector needs a dynamic parameter, then that parameter should probably be state in the store. If the parameter is not dynamic you can use a factory function like:

function expensiveItemFactory(price) {
  return createSelector(
    state => state.items,
    items => items.filter(item => item.value > price)
  )
}

function mapStateToProps(state) {
  return {
    shop: state.shop
  };
}

const expensive1 = expensiveItemFactory(200)
const expensive2 = expensiveItemFactory(400)

class anotherComponent extends Component {
  render() {
    return <p>Can you afford these? {expensive1(this.props.shop}</p>
  }
}

That said, I am willing to have my mind changed on this but so far it hasn't seemed useful enough to justify the extra complexity it would introduce.

@ellbee
Copy link
Collaborator

ellbee commented Oct 28, 2015

Oh, and thanks again for taking the time to make the PR. I am going to take another look at the answer to that FAQ as I don't think it is as clear as it should be.

@ellbee ellbee closed this Oct 28, 2015
@mdebbar
Copy link

mdebbar commented Mar 14, 2016

I guess I'm late to the party, but I'm already in love with Redux and Reselect. I've been thinking about creating dynamic selectors but couldn't find any good solution. I, though, have a very good use case for it. Say I have a list of all the books and a list of all the authors in my state:

var state = {
  books: [
    {id: 1, author_id: 1, ...},
    {id: 2, author_id: 2, ...},
    {id: 3, author_id: 1, ...},
    ...
  ],
  author: [
    {id: 1, name: "Author1", ...},
    {id: 2, name: "Author2", ...},
    ...
  ]
};

Different components on the page need to display books of different authors. So I need a selector that takes an author id and returns a list of books.

The best I could come up with was:

const allBooks = (state) => state.books;
const booksByAuthorId =
  (state, author_id) => allBooks(state).find(book => book.author_id === author_id);

But obviously this isn't memoized and could become very expensive when we have a lot of books.

@mdebbar
Copy link

mdebbar commented Mar 15, 2016

One way to do it, as you suggested, would be to keep the author_id in the state. In this case, we need to keep multiple author ids in the state and create a selector for each of them:

var state = {
  author_id_for_top_menu: 1,
  author_id_for_bottom_suggestions: 2,
  ...
  books: [
    {id: 1, author_id: 1, ...},
    {id: 2, author_id: 2, ...},
    {id: 3, author_id: 1, ...},
    ...
  ],
  author: [
    {id: 1, name: "Author1", ...},
    {id: 2, name: "Author2", ...},
    ...
  ]
};

const filterBooksByAuthorId =
  (books, author_id) => books.find(book => book.author_id === author_id);

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

const topMenuAuthorId = (state) => state.author_id_for_top_menu;
const booksForTopMenu = createSelector(
  [allBooks, topMenuAuthorId],
  filterBooksByAuthorId
);

const bottomSuggestionsAuthorId = (state) => state.author_id_for_bottom_suggestions;
const booksForBottomSuggestions = createSelector(
  [allBooks, bottomSuggestionsAuthorId],
  filterBooksByAuthorId
);

This works fine but it's a lot of redundant code. And of course it won't work if we have a dynamic number of components, each with a different author id.

@mdebbar
Copy link

mdebbar commented Mar 15, 2016

Looking at the Reselect code, I believe changing this line to:

return memoizedResultFunc(...params, ...args);

should allow dynamic parameters while correctly preserving memoization. The only issue is the size of the cache, which causes successive calls with different parameters to always result in cache miss:

booksByAuthorId(state, props, 1);
booksByAuthorId(state, props, 2); // cache miss
booksByAuthorId(state, props, 1); // cache miss

In this case, this suggestion can fix the cache miss issue.

Any thoughts @ellbee ?

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.

3 participants