Skip to content

Add design decisions to FAQ document #2528

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 8, 2017
Merged

Add design decisions to FAQ document #2528

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8c788c
add design decisions to FAQ document
sbakkila Jul 24, 2017
a71d918
Update DesignDecisions.md
sbakkila Jul 25, 2017
aad27f5
fix small typos
sbakkila Jul 25, 2017
5bf622f
fix: DesignDecision FAQ edits based on feedback
sbakkila Aug 7, 2017
0b496c2
fix: small changes to combineReducers question in DesignDecisions.md
sbakkila Aug 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions docs/FAQ.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@
- [Do I have to deep-clone my state in a reducer? Isn't copying my state going to be slow?](/docs/faq/Performance.md#performance-clone-state)
- [How can I reduce the number of store update events?](/docs/faq/Performance.md#performance-update-events)
- [Will having “one state tree” cause memory problems? Will dispatching many actions take up memory?](/docs/faq/Performance.md#performance-state-memory)
- **Design Decisions**
- [Why doesn't Redux pass the state and action to subscribers?](/docs/faq/DesignDecisions.md#does-not-pass-state-action-to-subscribers)
- [Why doesn't Redux support using classes for actions and reducers?](/docs/faq/DesignDecisions.md#does-not-support-classes)
- [Why does the middleware signature use currying?](/docs/faq/DesignDecisions.md#why-currying)
- [Why does applyMiddleware use a closure for dispatch?](/docs/faq/DesignDecisions.md#closure-dispatch)
- [Can you please change combineReducers to support nested state trees?](/docs/faq/DesignDecisions.md#combineReducers-limitations)
- [Why doesn't mapDispatchToProps allow use of return values from getState() or mapStateToProps()?](/docs/faq/DesignDecisions.md#no-asynch-in-mapDispatchToProps)
- **React Redux**
- [Why isn't my component re-rendering, or my mapStateToProps running?](/docs/faq/ReactRedux.md#react-not-rerendering)
- [Why is my component re-rendering too often?](/docs/faq/ReactRedux.md#react-rendering-too-often)
Expand Down
75 changes: 75 additions & 0 deletions docs/faq/DesignDecisions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Redux FAQ: Design Decisions

## Table of Contents

- [Why doesn't Redux pass the state and action to subscribers?](#does-not-pass-state-action-to-subscribers)
- [Why doesn't Redux support using classes for actions and reducers?](#does-not-support-classes)
- [Why does the middleware signature use currying?](#why-currying)
- [Why does applyMiddleware use a closure for dispatch?](#closure-dispatch)
- [Can you please change combineReducers to support nested state trees?](#combineReducers-limitations)
- [Why doesn't mapDispatchToProps allow use of return values from getState() or mapStateToProps()?](#no-asynch-in-mapDispatchToProps)


## Design Decisions

<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.
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

2.) the [virtual DOM equality check (vDOMEq)](https://facebook.github.io/react/docs/optimizing-performance.html#avoid-reconciliation)
3.) [React.PureComponent](https://facebook.github.io/react/docs/optimizing-performance.html#examples)
4.) Using React-Redux: use [mapStateToProps](https://github.com/reactjs/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options) to subscribe components to only the parts of the store that they need.

<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.
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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.
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


<a id="why-currying"></a>
### Why does the middleware signature use currying?
The [curried function signature](https://github.com/reactjs/redux/issues/1744) of declaring middleware is [deemed unnecessary](https://github.com/reactjs/redux/pull/784) by some, because both store and next are available when the applyMiddleware function is executed. This issue has been determined to not be [worth introducing breaking changes](https://github.com/reactjs/redux/issues/1744).

<a id="closure-dispatch"></a>
### Why does applyMiddleware use a closure for dispatch?
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?
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

No, but there are some limits to combineReducers that are worth knowing.
- combineReducers receives an object where the values are all reducer names. Additional nesting is not possible, which limits the state shape. The following code is not possible:
```
const rootReducer = combineReducers({
a : reducerA,
b : {
b1 : reducerB1,
b2 : reducerB2
}
});
```
- reducers within combineReducers are only passed the part of the state that they modify, and not the top state.

The default utility combineReducers is only one way to build a complex reducer. Consider using libraries like [combineSectionReducers](https://github.com/ryo33/combine-section-reducers) or [reduceReducers](https://github.com/acdlite/reduce-reducers) if you want your reducers to have a nested tree structure.

<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.
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


You cannot modify the state during the execution of mapStateToProps. Modifying the state from these functions could lead to infinite loops because every update would reinvoke the map functions. Calling getState inside mapStateToProps would always just return the same state that is passed to the function.

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
Copy link
Contributor

@markerikson markerikson Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

**Discussions**
* [#580: Why doesn't Redux pass the state to subscribers?](https://github.com/reactjs/redux/issues/580)
* [#2214: Alternate Proof of Concept: Enhancer Overhaul -- more on debouncing](https://github.com/reactjs/redux/pull/2214)

* [#1171: Why doesn't Redux use classes for actions and reducers?](https://github.com/reactjs/redux/issues/1171#issuecomment-196819727)
* Why does the middleware signature use currying?
* See - [#55](https://github.com/reactjs/redux/pull/55), [#534](https://github.com/reactjs/redux/issues/534), [#784](https://github.com/reactjs/redux/pull/784), [#922](https://github.com/reactjs/redux/issues/922), [#1744](https://github.com/reactjs/redux/issues/1744)
* Why does applyMiddleware use a closure for dispatch?
* See - [#1592](https://github.com/reactjs/redux/pull/1592) and [#2097](https://github.com/reactjs/redux/issues/2097)
* [#1768 Can you please change combineReducers to support nested state trees?](https://github.com/reactjs/redux/pull/1768)
* [#237 Why doesn't mapDispatchToProps allow use of return values from getState() or mapStateToProps()?](https://github.com/reactjs/react-redux/issues/237)