Is it bad practice to just have a `stores` directory? #1436

Closed
lmatteis opened this Issue Feb 23, 2016 · 8 comments

Comments

2 participants
@lmatteis

I just started using Redux and I found myself using this structure:

A stores directory with an index.js as follows:

import { createStore } from 'redux'

export const currentPageStore = createStore((state, action) => {
  switch (action.type) {
  case 'SET_CURRENT_PAGE':
    return {
      page: action.page,
      hashHex: action.hashHex
    }
  default:
    return state
  }
})

export const tweetsStore = createStore((state, action) => {
  switch (action.type) {
  case 'RESET':
    return 'RESET'
  case 'ADD_TWEET':
    return action.tweet
  default:
    return state
  }
})

Then I have a components with my React components that use this store such as:

import { tweetsStore, currentPageStore } from '../stores'

export default class Main extends Component {
  constructor(props) {
    super(props)
    this.state = {
      tweets: []
    }

    this.unsubscribe = tweetsStore.subscribe(() => {
      var s = tweetsStore.getState()
      if (s == 'RESET')
        return this.setState({ tweets: [] })

      this.setState((state) => { tweets: state.tweets.push(s) })
    })
  }

And when I want to change the store I simply call dispatch:

var action = {
  type: 'ADD_TWEET',
  tweet: {
    foo: foo
  }
}
tweetsStore.dispatch(action)

As you can see I simply have a single tweetStore variable to think about and I don't have to deal with importing actions from their appropriate actions directory.

I'm wondering whether this is bad practice or not.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 23, 2016

Collaborator

You seem to be asking about three different things at once.

  1. We don’t encourage you to have multiple stores in Redux unless you have very good reasons to separate them. Logical separation is not such a reason: you should use reducer composition instead and keep it as a single store with a single root reducer that calls other reducers.
  2. That said you can keep store as a singleton you import from a module. We don’t recommend this because it makes it harder to convert the app to be server rendered if you ever so desire. Singleton stores don’t work well on the server because usually you want to isolate data for every request and perform some asynchronous data fetching. Therefore you would want to have a store per request on the server. However technically you can keep using this pattern if you are sure you are never going to need server rendering. Also, using subscribe() directly is a worse idea than using connect() from React Redux. connect() includes many optimizations that are hard to write by hand so if you care about performance you should probably use it instead.
  3. You can absolutely dispatch actions inline from components if you wish so. Action creators is just a method of code organization we recommend for various reasons but you are not obliged to use it. You might find that in bigger apps action creators are helpful for making sure all action objects of the same type are structured the same way and for centralizing side effects but indeed it may be unnecessary in smaller apps.

Finally, your use of setState() is not clear to me. Why do you put action in state and then read it in the component? Why not return an empty array from the reducer instead? Why call push (mutation!) rather than dispatch an action?

I would suggest you to watch my free Egghead course: https://egghead.io/series/getting-started-with-redux. Please don’t skip lessons ;-). It should answer your questions and show a more idiomatic way of building apps with Redux.

Collaborator

gaearon commented Feb 23, 2016

You seem to be asking about three different things at once.

  1. We don’t encourage you to have multiple stores in Redux unless you have very good reasons to separate them. Logical separation is not such a reason: you should use reducer composition instead and keep it as a single store with a single root reducer that calls other reducers.
  2. That said you can keep store as a singleton you import from a module. We don’t recommend this because it makes it harder to convert the app to be server rendered if you ever so desire. Singleton stores don’t work well on the server because usually you want to isolate data for every request and perform some asynchronous data fetching. Therefore you would want to have a store per request on the server. However technically you can keep using this pattern if you are sure you are never going to need server rendering. Also, using subscribe() directly is a worse idea than using connect() from React Redux. connect() includes many optimizations that are hard to write by hand so if you care about performance you should probably use it instead.
  3. You can absolutely dispatch actions inline from components if you wish so. Action creators is just a method of code organization we recommend for various reasons but you are not obliged to use it. You might find that in bigger apps action creators are helpful for making sure all action objects of the same type are structured the same way and for centralizing side effects but indeed it may be unnecessary in smaller apps.

Finally, your use of setState() is not clear to me. Why do you put action in state and then read it in the component? Why not return an empty array from the reducer instead? Why call push (mutation!) rather than dispatch an action?

I would suggest you to watch my free Egghead course: https://egghead.io/series/getting-started-with-redux. Please don’t skip lessons ;-). It should answer your questions and show a more idiomatic way of building apps with Redux.

@gaearon gaearon closed this Feb 23, 2016

@lmatteis

This comment has been minimized.

Show comment
Hide comment
@lmatteis

lmatteis Feb 23, 2016

Finally, your use of setState() is not clear to me. Why do you put action in state and then read it in the component? Why not return an empty array from the reducer instead? Why call push (mutation!) rather than dispatch an action?

Doesn't Flux say that your view should listen to store alterations and then change its own state accordingly? Why does it matter if the change is a mutation or a complete object replace? I thought the entire idea behind immutability of Redux was that you could avoid calling emitChange() in your stores and just check whether the data is different, but that should be in the reducers, and not in the setState(). And in fact I don't mutate stuff in the reducers.

Finally, your use of setState() is not clear to me. Why do you put action in state and then read it in the component? Why not return an empty array from the reducer instead? Why call push (mutation!) rather than dispatch an action?

Doesn't Flux say that your view should listen to store alterations and then change its own state accordingly? Why does it matter if the change is a mutation or a complete object replace? I thought the entire idea behind immutability of Redux was that you could avoid calling emitChange() in your stores and just check whether the data is different, but that should be in the reducers, and not in the setState(). And in fact I don't mutate stuff in the reducers.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 23, 2016

Collaborator

Doesn't Flux say that your view should listen to store alterations and then change its own state accordingly? Why does it matter if the change is a mutation or a complete object replace? I thought the entire idea behind immutability of Redux was that you could avoid calling emitChange() in your stores and just check whether the data is different, but that should be in the reducers, and not in the setState().

This is all correct. However the code your posted does not seem to work as you describe.

And in fact I don't mutate stuff in the reducers.

You don’t, but you mutate the local state in the component. I don’t understand why you are doing it.

Collaborator

gaearon commented Feb 23, 2016

Doesn't Flux say that your view should listen to store alterations and then change its own state accordingly? Why does it matter if the change is a mutation or a complete object replace? I thought the entire idea behind immutability of Redux was that you could avoid calling emitChange() in your stores and just check whether the data is different, but that should be in the reducers, and not in the setState().

This is all correct. However the code your posted does not seem to work as you describe.

And in fact I don't mutate stuff in the reducers.

You don’t, but you mutate the local state in the component. I don’t understand why you are doing it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 23, 2016

Collaborator

Here is an idiomatic Redux take on your code.

I kept store a singleton (although as I said we don’t encourage that) but used a single store and fixed tweets reducer to actually handle the RESET action rather than return it.

store.js

import { createStore, combineReducers } from 'redux'

const currentPage = (state = { page: 0 }, action) => {
  switch (action.type) {
  case 'SET_CURRENT_PAGE':
    return {
      page: action.page,
      hashHex: action.hashHex
    }
  default:
    return state
  }
}

const tweets = (state = [], action) => { // note: initial state is []
  switch (action.type) {
  case 'RESET':
    return [] // reset basically just forces reducer to return [] again
  case 'ADD_TWEET':
    return [ ...state, action.tweet ] // don't need push() anywhere, return a new array
  default:
    return state
  }
}

const reducer = combineReducers({ // rather than use multiple stores, combine reducers
  currentPage,
  tweets
})

export default createStore(reducer)

component.js

import store from '../store'

export default class Main extends Component {
  constructor(props) {
    super(props)
    this.state = store.getState() // use store state as the source of truth
  }

  componentDidMount() {
    this.unsubscribe = store.subscribe(() => { // don't subscribe until you're mounted
      this.setState(store.getState()) // use store state as source of truth (optionally: select what you need here)
    })
  }

  componentWillUnmount() {
    if (this.unsubscribe) { // don't forget to unsubscribe when unmounting
      this.unsubscribe()
      this.unsubscribe = null
    }
  }
}

Note that component no longer has any access to the action itself—it should render predictable based on the current state. It also contains no special logic to push something into the local todos state, and instead it considers store the source of truth. Hope this helps, and if it’s not clear, please do watch the videos because they cover exactly this material.

Collaborator

gaearon commented Feb 23, 2016

Here is an idiomatic Redux take on your code.

I kept store a singleton (although as I said we don’t encourage that) but used a single store and fixed tweets reducer to actually handle the RESET action rather than return it.

store.js

import { createStore, combineReducers } from 'redux'

const currentPage = (state = { page: 0 }, action) => {
  switch (action.type) {
  case 'SET_CURRENT_PAGE':
    return {
      page: action.page,
      hashHex: action.hashHex
    }
  default:
    return state
  }
}

const tweets = (state = [], action) => { // note: initial state is []
  switch (action.type) {
  case 'RESET':
    return [] // reset basically just forces reducer to return [] again
  case 'ADD_TWEET':
    return [ ...state, action.tweet ] // don't need push() anywhere, return a new array
  default:
    return state
  }
}

const reducer = combineReducers({ // rather than use multiple stores, combine reducers
  currentPage,
  tweets
})

export default createStore(reducer)

component.js

import store from '../store'

export default class Main extends Component {
  constructor(props) {
    super(props)
    this.state = store.getState() // use store state as the source of truth
  }

  componentDidMount() {
    this.unsubscribe = store.subscribe(() => { // don't subscribe until you're mounted
      this.setState(store.getState()) // use store state as source of truth (optionally: select what you need here)
    })
  }

  componentWillUnmount() {
    if (this.unsubscribe) { // don't forget to unsubscribe when unmounting
      this.unsubscribe()
      this.unsubscribe = null
    }
  }
}

Note that component no longer has any access to the action itself—it should render predictable based on the current state. It also contains no special logic to push something into the local todos state, and instead it considers store the source of truth. Hope this helps, and if it’s not clear, please do watch the videos because they cover exactly this material.

@gaearon gaearon added the question label Feb 23, 2016

@lmatteis

This comment has been minimized.

Show comment
Hide comment
@lmatteis

lmatteis Feb 23, 2016

Interesting. I might be used too much to Facebook's Flux to understand this, but isn't the Main component now listening on all stores? So for instance if the 'SET_CURRENT_PAGE' action is dispatched, my Main component will set its state to the object, even though it doesn't care about it at all. I guess your comment (optionally: select what you need here) allows for some selection to happen, but how? Based on the action.type?

Thanks for clearing things up.

Interesting. I might be used too much to Facebook's Flux to understand this, but isn't the Main component now listening on all stores? So for instance if the 'SET_CURRENT_PAGE' action is dispatched, my Main component will set its state to the object, even though it doesn't care about it at all. I guess your comment (optionally: select what you need here) allows for some selection to happen, but how? Based on the action.type?

Thanks for clearing things up.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 23, 2016

Collaborator
import store from '../store'

const mapStateToLocalState = (state) => ({
  todos: state.todos
})

export default class TodoList extends Component {
  constructor(props) {
    super(props)
    this.state = mapStateToLocalState(store.getState())
  }

  componentDidMount() {
    this.unsubscribe = store.subscribe(() => {
      this.setState(mapStateToLocalState(store.getState())))
    })
  }

  componentWillUnmount() {
    if (this.unsubscribe) {
      this.unsubscribe()
      this.unsubscribe = null
    }
  }
}

But at this point you will be reinventing React Redux which generates container components for you:

import { connect } from 'react-redux'

const mapStateToProps = (state) => ({
  todos: state.todos
})

class TodoList extends Component {
  render() {
    return <ul>{this.props.todos.map(todo => <li>{todo.text}</li>)}</ul>
  }
}

export default connect(
  mapStateToProps
)(TodoList)
Collaborator

gaearon commented Feb 23, 2016

import store from '../store'

const mapStateToLocalState = (state) => ({
  todos: state.todos
})

export default class TodoList extends Component {
  constructor(props) {
    super(props)
    this.state = mapStateToLocalState(store.getState())
  }

  componentDidMount() {
    this.unsubscribe = store.subscribe(() => {
      this.setState(mapStateToLocalState(store.getState())))
    })
  }

  componentWillUnmount() {
    if (this.unsubscribe) {
      this.unsubscribe()
      this.unsubscribe = null
    }
  }
}

But at this point you will be reinventing React Redux which generates container components for you:

import { connect } from 'react-redux'

const mapStateToProps = (state) => ({
  todos: state.todos
})

class TodoList extends Component {
  render() {
    return <ul>{this.props.todos.map(todo => <li>{todo.text}</li>)}</ul>
  }
}

export default connect(
  mapStateToProps
)(TodoList)
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 23, 2016

Collaborator

For now this is all I can answer here. All of this is available in our docs at http://redux.js.org, and in the videos at https://egghead.io/series/getting-started-with-redux. I would really appreciate if you considered looking through them as they answer your questions.

Collaborator

gaearon commented Feb 23, 2016

For now this is all I can answer here. All of this is available in our docs at http://redux.js.org, and in the videos at https://egghead.io/series/getting-started-with-redux. I would really appreciate if you considered looking through them as they answer your questions.

@lmatteis

This comment has been minimized.

Show comment
Hide comment
@lmatteis

lmatteis Feb 23, 2016

Will do thanks a bunch!

Will do thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment