Add design decisions to FAQ document #2528

Merged
merged 5 commits into from Aug 8, 2017

Conversation

3 participants
@sbakkila
Contributor

sbakkila commented Jul 24, 2017

I wrote up some answers for design decisions to the FAQ in response to issue #1785

I don't think that the md is perfect, and my page isn't showing up when I build the gitbook, but I wasn't sure how to proceed. Please advise on this. Thanks!

This is my first open source contribution. 馃槃

@sbakkila sbakkila referenced this pull request Jul 24, 2017

Closed

FAQ updates #1785

@sbakkila sbakkila changed the title from add design decisions to FAQ document to Add design decisions to FAQ document Jul 24, 2017

docs/faq/DesignDecisions.md
+
+<a id="does-not-pass-state-action-to-subscribers"></a>
+### Why doesn't Redux pass the state and action to subscribers?
+Subscribers are intended to respond to the state value itself, not the action. Updates to the state are not always processed synchronously, because Redux sometimes processes actions in batches to optimize performance and repeated re-rendering. The intended guarantee is that Redux eventually calls all subscribers with the most recent state, but not that it always calls each subscriber for each action. The store state is available in the subscriber simply by calling store.getState(). The action cannot be made available in the subsribers without breaking the way that actions are batched.

This comment has been minimized.

@timdorr

timdorr Jul 25, 2017

Member

There is no batching in Redux natively. However, you can add that functionality. I might reword it to say "because libraries can change Redux to process actions in batches".

@timdorr

timdorr Jul 25, 2017

Member

There is no batching in Redux natively. However, you can add that functionality. I might reword it to say "because libraries can change Redux to process actions in batches".

This comment has been minimized.

@sbakkila

sbakkila Jul 25, 2017

Contributor

Thanks, good catch! Updated.

@sbakkila

sbakkila Jul 25, 2017

Contributor

Thanks, good catch! Updated.

@sbakkila

This comment has been minimized.

Show comment
Hide comment
@sbakkila

sbakkila Jul 29, 2017

Contributor

@markerikson @timdorr I just wanted to check again for any needed revisions. I have time to work on it this weekend if you have any suggestions! Thanks!

Contributor

sbakkila commented Jul 29, 2017

@markerikson @timdorr I just wanted to check again for any needed revisions. I have time to work on it this weekend if you have any suggestions! Thanks!

docs/faq/DesignDecisions.md
+
+<a id="does-not-pass-state-action-to-subscribers"></a>
+### Why doesn't Redux pass the state and action to subscribers?
+Subscribers are intended to respond to the state value itself, not the action. Updates to the state are not always processed synchronously, because libraries can change Redux to process actions in batches to optimize performance and avoid repeated re-rendering. The intended guarantee is that Redux eventually calls all subscribers with the most recent state, but not that it always calls each subscriber for each action. The store state is available in the subscriber simply by calling store.getState(). The action cannot be made available in the subsribers without breaking the way that actions are batched.

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

Let's rephrase this a bit. The actual state updates are always processed synchronously. However, enhancers can override store.dispatch to change the way subscribers are notified. A couple examples are https://github.com/manaflair/redux-batch , which allows passing an array of actions to dispatch() with only one notification, and https://github.com/tappleby/redux-batched-subscribe , which can debounce notifications. So, it would be good to clarify the distinction in behavior there.

Two other tweaks here: "subsribers" -> "subscribers", and let's add a link to the perf FAQ entry on minimizing update events.

@markerikson

markerikson Jul 29, 2017

Contributor

Let's rephrase this a bit. The actual state updates are always processed synchronously. However, enhancers can override store.dispatch to change the way subscribers are notified. A couple examples are https://github.com/manaflair/redux-batch , which allows passing an array of actions to dispatch() with only one notification, and https://github.com/tappleby/redux-batched-subscribe , which can debounce notifications. So, it would be good to clarify the distinction in behavior there.

Two other tweaks here: "subsribers" -> "subscribers", and let's add a link to the perf FAQ entry on minimizing update events.

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

Also: add inline code backticks for store.getState()

@markerikson

markerikson Jul 29, 2017

Contributor

Also: add inline code backticks for store.getState()

docs/faq/DesignDecisions.md
+Subscribers are intended to respond to the state value itself, not the action. Updates to the state are not always processed synchronously, because libraries can change Redux to process actions in batches to optimize performance and avoid repeated re-rendering. The intended guarantee is that Redux eventually calls all subscribers with the most recent state, but not that it always calls each subscriber for each action. The store state is available in the subscriber simply by calling store.getState(). The action cannot be made available in the subsribers without breaking the way that actions are batched.
+
+A potential use-case for using the action inside a subscriber -- which is an unsupported feature -- is to ensure that a component only re-renders after certain kinds of actions. Re-rendering should instead be controlled instead through:
+1.) the [shouldComponentUpdate](https://facebook.github.io/react/docs/react-component.html#shouldcomponentupdate) lifecycle method

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

The list here isn't formatted right. Removing the parens and just having 1. should fix it.

@markerikson

markerikson Jul 29, 2017

Contributor

The list here isn't formatted right. Removing the parens and just having 1. should fix it.

docs/faq/DesignDecisions.md
+### Why doesn't Redux support using classes for actions and reducers?
+The pattern of using functions, called action creators, to return action objects may seem counterintuitive to programmers with a lot of Object Oriented Programming experience, who would see this is a strong use-case for Classes and instances. Class instances for action objects and reducers are not supported because class instances make serialization and deserialization tricky. Deserialization methods like JSON.parse(string) will return a plain old Javascript object rather than class instances.
+
+Serialization enables the brower to store all actions that have been dispatched, as well as the previous store states, with much less memory. Rewinding and 'hot reloading' the store is central to the Redux developer experience and the function of Redux DevTools. This also enables deserialized actions to be stored on the server and re-serialized in the brower in the case of server-side rendering with Redux.

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

Add a link to the store FAQ entry on serializable values in the store.

@markerikson

markerikson Jul 29, 2017

Contributor

Add a link to the store FAQ entry on serializable values in the store.

docs/faq/DesignDecisions.md
+
+<a id="does-not-support-classes"></a>
+### Why doesn't Redux support using classes for actions and reducers?
+The pattern of using functions, called action creators, to return action objects may seem counterintuitive to programmers with a lot of Object Oriented Programming experience, who would see this is a strong use-case for Classes and instances. Class instances for action objects and reducers are not supported because class instances make serialization and deserialization tricky. Deserialization methods like JSON.parse(string) will return a plain old Javascript object rather than class instances.

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

General request: add backticks for inline code formatting to all mentions of function names and stuff like JSON.parse().

@markerikson

markerikson Jul 29, 2017

Contributor

General request: add backticks for inline code formatting to all mentions of function names and stuff like JSON.parse().

docs/faq/DesignDecisions.md
+applyMiddleware takes the existing dispatch from the store and closes over it to create the initial chain of middlewares that have been invoked with an object that exposes the getState and dispatch functions, which enables middlewares that [rely on dispatch during initialization](https://github.com/reactjs/redux/pull/1592) to run.
+
+<a id="combineReducers-limitations"></a>
+### Can you please change combineReducers to support nested state trees?

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

Let's change the title on this one. Maybe something like: "Why doesn't combineReducers include a third argument with the entire state?"

I'd also like to see the answer fleshed out a bit more. Overall, the general answer is that combineReducers is deliberately opinionated, and it's also not immediately obvious what a potential third arg should be (entire state tree, some callback, some other part of the state tree, ... ?). We might change that in the future, but for now the answer is "if it doesn't fit your use case, you can write your own".

@markerikson

markerikson Jul 29, 2017

Contributor

Let's change the title on this one. Maybe something like: "Why doesn't combineReducers include a third argument with the entire state?"

I'd also like to see the answer fleshed out a bit more. Overall, the general answer is that combineReducers is deliberately opinionated, and it's also not immediately obvious what a potential third arg should be (entire state tree, some callback, some other part of the state tree, ... ?). We might change that in the future, but for now the answer is "if it doesn't fit your use case, you can write your own".

docs/faq/DesignDecisions.md
+
+The preferred way to handle this use-case (needing to alter props based on the current state and mapDispatchToProps functions) is to work from mergeProps, the third argument to the connect function. If specified, it is passed the result of mapStateToProps(), mapDispatchToProps(), and the container component's props. The plain object returned from mergeProps will be passed as props to the wrapped component.
+
+#### Further information

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

I'd like these links moved to each be at the end of the appropriate question/answer.

@markerikson

markerikson Jul 29, 2017

Contributor

I'd like these links moved to each be at the end of the appropriate question/answer.

docs/faq/DesignDecisions.md
+
+<a id="no-asynch-in-mapDispatchToProps"></a>
+### Why doesn't mapDispatchToProps allow use of return values from getState() or mapStateToProps()?
+In general, connect generates a props object out of a closure that is injected with both the current state and dispatch. Asynchronous logic does not belong in the mapStateToProps and mapDispatchToProps functions at all. They should be only pure functions which transform the state to props and bind action creators to dispatch.

This comment has been minimized.

@markerikson

markerikson Jul 29, 2017

Contributor

Not sure I understand/follow the statements in these first two paragraphs.

I don't think we've really ever had anyone ask about calling getState inside of a mapStateToProps function. The typical request/use case has been wanting to use either the entire state or the return value of mapState inside of mapDispatch, so that when functions are declared inside of mapDispatch, they can close over the latest returned values from the store. We don't allow that approach in mapDispatch because it would mean also calling mapDispatch every time the store is updated, and also cause re-creation of functions every time, thus adding a lot of perf overhead.

As the third paragraph says, if you really want to do that, you can provide a mergeProps function and implement this behavior yourself, but we heavily discourage that approach. We would much rather encourage use of component methods that pass props to bound action creators, like this.props.toggleTodo(this.props.todoId). (And, as my own personal preference, you shouldn't even write an actual mapDispatch function at all - instead, write separate action creator functions, and use the object shorthand form of mapDispatch to bind them automatically.)

Would appreciate a rework of this answer.

@markerikson

markerikson Jul 29, 2017

Contributor

Not sure I understand/follow the statements in these first two paragraphs.

I don't think we've really ever had anyone ask about calling getState inside of a mapStateToProps function. The typical request/use case has been wanting to use either the entire state or the return value of mapState inside of mapDispatch, so that when functions are declared inside of mapDispatch, they can close over the latest returned values from the store. We don't allow that approach in mapDispatch because it would mean also calling mapDispatch every time the store is updated, and also cause re-creation of functions every time, thus adding a lot of perf overhead.

As the third paragraph says, if you really want to do that, you can provide a mergeProps function and implement this behavior yourself, but we heavily discourage that approach. We would much rather encourage use of component methods that pass props to bound action creators, like this.props.toggleTodo(this.props.todoId). (And, as my own personal preference, you shouldn't even write an actual mapDispatch function at all - instead, write separate action creator functions, and use the object shorthand form of mapDispatch to bind them automatically.)

Would appreciate a rework of this answer.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jul 29, 2017

Contributor

@sbakkila : Thanks for working on this! Left several requests for changes, but this is definitely on the right track. Appreciate it!

Contributor

markerikson commented Jul 29, 2017

@sbakkila : Thanks for working on this! Left several requests for changes, but this is definitely on the right track. Appreciate it!

@sbakkila

This comment has been minimized.

Show comment
Hide comment
@sbakkila

sbakkila Aug 7, 2017

Contributor

@markerikson thanks for the feedback! I have made the suggested edits, and pushed changes to my fork. Let me know if you see anything else for me to fix.

Contributor

sbakkila commented Aug 7, 2017

@markerikson thanks for the feedback! I have made the suggested edits, and pushed changes to my fork. Let me know if you see anything else for me to fix.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Aug 8, 2017

Contributor

Tell you what. I see a couple more nitpicky things I'd like to tweak, but I think this is sufficiently good for now, so I'll go ahead and merge it.

Thank you VERY much for contributing this section, and working with me to improve it. I still need to go back to the "FAQ Updates" thread and figure out what other stuff still needs to be added, but if you see anything else that's not in the FAQ yet, please feel free to start adding those too.

Contributor

markerikson commented Aug 8, 2017

Tell you what. I see a couple more nitpicky things I'd like to tweak, but I think this is sufficiently good for now, so I'll go ahead and merge it.

Thank you VERY much for contributing this section, and working with me to improve it. I still need to go back to the "FAQ Updates" thread and figure out what other stuff still needs to be added, but if you see anything else that's not in the FAQ yet, please feel free to start adding those too.

@markerikson markerikson merged commit 7d1cabd into reduxjs:master Aug 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment