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

Selector dependencies using this #14

Open
acjay opened this issue Mar 1, 2017 · 7 comments
Open

Selector dependencies using this #14

acjay opened this issue Mar 1, 2017 · 7 comments

Comments

@acjay
Copy link
Contributor

acjay commented Mar 1, 2017

Right now, RRC provides no real help for the selector story. In our use case at Artsy, we have a great deal of selectors, which depend on the state and on each other. However, writing these selectors as functions from state to a derived value is a bit awkward, composing them is even more so, and using something like reselect for memoization is so awkward that we don't even bother.

In the example, we have:

import { PropTypes } from 'react'

export const selectedReddit = state => state.selectedReddit
selectedReddit.propType = PropTypes.string.isRequired

export const postsByReddit = state => state.postsByReddit
postsByReddit.propType = PropTypes.object.isRequired

export const posts = state => {
  const p = state.postsByReddit[selectedReddit(state)]
  return p && p.items ? p.items : []
}
posts.propType = PropTypes.array.isRequired

export const isFetching = state => {
  const posts = state.postsByReddit[selectedReddit(state)]
  return posts ? posts.isFetching : true
}
isFetching.propType = PropTypes.bool.isRequired

export const lastUpdated = state => {
  const posts = state.postsByReddit[selectedReddit(state)]
  return posts && posts.lastUpdated
}
lastUpdated.propType = PropTypes.number

The functional composition works, but it's also pretty noisy. A better API might be:

export class Selectors {
  get posts() {
    const p = this.postsByReddit[this.selectedReddit]
    return p && p.items ? p.items : []
  }

  get isFetching() {
    const posts = this.postsByReddit[this.selectedReddit]
    return posts ? posts.isFetching : true
  }

  get lastUpdated() {
    const posts = this.postsByReddit[this.selectedReddit]
    return posts && posts.lastUpdated
  }
}

Selectors.propTypes = {
  selectedReddit: PropTypes.string.isRequired,
  postsByReddit: PropTypes.object.isRequired,
  posts: PropTypes.array.isRequired,
  isFetching: PropTypes.bool.isRequired,
  lastUpdated: PropTypes.number
};

This feels much more direct and intuitive. Library magic would make sure that the Redux state is accessible via this.

Selectors would also become lazy, instead of eagerly computed. It may also be possible to rely on said library magic to opt selectors into memoization.

@acjay
Copy link
Contributor Author

acjay commented Mar 1, 2017

It would also be interesting to see if there would be a good way to apply PropTypes in-line with the selector methods, as annotations. Then we wouldn't need a class to attach the metadata to. Something like:

export default {
  @selector(posts: PropTypes.array.isRequired) 
  get posts() {
    const p = this.postsByReddit[this.selectedReddit]
    return p && p.items ? p.items : []
  },

  // ....

Without a final syntax for decorators, I'm not sure this is feasible. But even if we had a good way to apply these annotations, PropTypes would still need to be specified for the raw state properties. Maybe then, the idea would be to define selectors for raw state properties, and the raw state properties wouldn't be accessible on this, but on this.state instead:

export default {
  @selector(PropTypes.string.isRequired)
  get selectedRedit() { return this.state.selectedReddit },
  
  @selector(PropTypes.object.isRequired)
  get postsByReddit() { return this.state.postsByReddit },

  @selector(PropTypes.array.isRequired) 
  get posts() {
    const p = this.postsByReddit[this.selectedReddit]
    return p && p.items ? p.items : []
  },

  @selector(PropTypes.bool.isRequired) 
  get isFetching() { 
    const posts = this.postsByReddit[this.selectedReddit];
    return posts ? posts.isFetching : true  
  },

  @selector(PropTypes.number.isRequired) 
  get lastUpdated() { 
    const posts = this.postsByReddit[this.selectedReddit];
    return posts && posts.lastUpdated 
  },
}

@damassi
Copy link
Member

damassi commented Mar 1, 2017

This does feel much more intuitive. One issue I'd like to raise in terms of API design is first-class support for reselect since it's a) so widely used; and b) so powerful. Without the ability to easily opt-in to using reselect (through the users own implementation -- meaning, being able to seamlessly swap out a RRC selector for a reselect selector) I think we would be creating a slight disadvantage.

@acjay
Copy link
Contributor Author

acjay commented Mar 2, 2017

Yep, I definitely want memoization to be available. But our past experience with Reselect was that it was pretty awkward to apply -- lots of boilerplate. Don't get me wrong, we'd still use it if we knew we had a selector that was killing performance, but I think it's best applied sparingly.

I'd like to try to provide that functionality with a cleaner API, whether that uses Reselect under the hood, or some other memoization implementation. I also find MobX to be pretty fascinating, and everything I've read says it's pretty robust. Maybe it would be possible to use its observables and computed values directly to implement this.

If we did use Reselect though, it would probably be important to make it opt-in, and to expose its configurability.

@damassi
Copy link
Member

damassi commented Mar 2, 2017

When you were using reselect before what kind of boilerplate were you running into? I've used it extensively (in a more vanilla redux setup) and have never run into anything that was more complex than a function call -- curious how you were using it?

Something like...

import { createSelector } from 'reselect'

const nameSelector = state => state.name
const ageSelector = state => state.age

const mapStateToProps = createSelector(nameSelector, ageSelector,
  (name, age) => {
     return name + ' ' + age
  }
)

// Or

const mapStateToProps = createSelector(
  state => state.name,
  state => state.age,

  (name, age) => {
    return name + ' ' + age 
  }
)

Now that I've typed it out I can definitely see how this could be a little boilerplate heavy in an app like Prediction, but I can also see it being seamlessly (and simply) incorporated behind RRC.

One other thing to possibly look into is react-redux's advanced connect options (scroll down). Seems like there's a lot of possibility there for optimizations.

Will check out MobX! I've heard of it but have never dove in.

@acjay
Copy link
Contributor Author

acjay commented Mar 2, 2017

Yeah, exactly. With the probably several dozen selectors we have over there, createSelector dance really gummed things up. There were a whole bunch of new names floating around every selector for the calculated values of its selector dependencies. Like, we'd call nameSelector -> name, because it could very well be accessed by other things than the derived selector. But then we'd want to call that argument something else so as not to shadow name. It was unwieldy.

So, we tried doing without memoization and didn't notice a difference at all. I haven't collected hard data on it, but I'd guess that calling every selector (as happens on every state change in RRC) doesn't actually take more than maybe a couple milliseconds. In your example, at the two function calls have to happen to determine whether to concatenate some strings, combined with the internal machinery of Reselect. This could actually be a net loss versus simply concatenating the strings every time. A lot of our selectors actually are pretty much like this.

My concern with Reselect is that it's designed to work with selectors designed as functions over the state. In a paradigm where selectors access the state on this (either mixed directly in or on a state property), I'm not sure Reselect can be used very directly. And that might be ok, it's not so magical that we can't just adapt its logic to this use case.

RRC today computes all the selectors eagerly every store change. This is wasteful in the case that only some selectors are used, or in the case that most of the selector outputs didn't change -- probably the norm. Naively, this proposal would lead to selectors that are recalculated on demand. But this is wasteful when one selector is accessed many times by other selectors, components or controller methods. We could avoid these problems by only recomputing a selector when its inputs change. (We could go even further and trade memory for time by caching some number of selector input * output combinations.)

But this comes at a certain cost of needing to specify exactly what the dependencies of each selector is. I haven't figured yet out how MobX computed seems to get around this.

@acjay
Copy link
Contributor Author

acjay commented Mar 2, 2017

I looked into how MobX computed works. There's an in-depth discussion of it here.

If I understand it, the TLDR of it is that computed properties don't statically track their depedencies (i.e. Observables). Instead, the pull values from their dependencies on-demand. This triggers a subscription to those dependencies, so that upstream changes propagate downward going forward. Extremely clever!

I'm very intrigued by this. Since Object.assign invokes setters, we could simply do:

export class Selectors {
  @observable selectedReddit;
  @observable postsByReddit;

  @computed get posts() {
    const p = this.postsByReddit[this.selectedReddit]
    return p && p.items ? p.items : []
  }

  @computed get isFetching() {
    const posts = this.postsByReddit[this.selectedReddit]
    return posts ? posts.isFetching : true
  }

  @computed get lastUpdated() {
    const posts = this.postsByReddit[this.selectedReddit]
    return posts && posts.lastUpdated
  }
}

Selectors.propTypes = {
  selectedReddit: PropTypes.string.isRequired,
  postsByReddit: PropTypes.object.isRequired,
  posts: PropTypes.array.isRequired,
  isFetching: PropTypes.bool.isRequired,
  lastUpdated: PropTypes.number
};

RRC would simply instantiate your selector class one time and Object.assign the state to it every store change. I think this would work whether or not the consumer chose to use the MobX decorators, and the dependency on MobX would exist only in client code.

@plandem
Copy link

plandem commented Mar 22, 2017

any news? :)

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

No branches or pull requests

3 participants