-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
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
Structuring reducers page #1930
Structuring reducers page #1930
Conversation
Note that a _lot_ of this content duplicates the existing "Reducers" doc page, which bothers me, but we'll work that out later.
Really emphasize the prerequisites
Fixes a typo
|
||
**Key Concepts**: | ||
|
||
- Splitting relational/nested data up into separate tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it it clear what "tables" refers to here. Am I right that this item means that the reader should understand database normalization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, since that is the suggested structure for organizing nested/relational data in a Redux store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, thanks for taking the time to review this!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! Databases haven't been mentioned yet in this doc is all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the "Normalizing data" pages :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I mean that from the point of view of someone reading this doc for the first time there has been no mention of databases by the time they get to this mention of tables. I was just wondering if people are going to be like "huh, what's a JavaScript table?". I mean, they won't but they might not jump to database tables immediately either. I dunno, the more I talk about it the less important it seems 😄
Am I right that this item means that the reader should understand database normalization?
I asked this because I wasn't clear if you were saying that a reader would have to specifically understand database normalization to be able to read the Normalizing Data section. Having read that section now, I am not sure they do. In fact, I am not sure that a reader has to understand any of the concepts listed as prerequisites for Normalizing Data prior to reading it, they are thoroughly explained in the section itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my first attempt at writing a couple of these sections, I found myself totally trying to explain concepts like "immutable data" from the ground up, and realized I was totally wasting effort re-explaining stuff that was better written and explained elsewhere. I did do some explanation of normalization concepts and terms in those pages, but didn't go fully in depth.
Any suggestions for better handling the "prereqs" aspect? Some way to better tie it in to the rest of the writing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked this because I wasn't clear if you were saying that a reader would have to specifically understand database normalization to be able to read the Normalizing Data section.
Interesting related discussion at choojs/choo#252 (comment). In particular:
At first blush, the idea of "importing" relations or graph structures, wholesale, kind of turns my stomach. Either would bring with it a whole mound of concepts dwarfing all the other fundamental primitives in choo combined. "If you wish to make a choo app from scratch, you must first understand relational theory?"
@markerikson I'm about 2/3rds of the way through taking notes on these. I agree with @jimbolla that some kind of executive summary might be useful to make sure the key ideas stand out. One consistent nitpick I've noticed is that there are often 2 spaces after full stops, while the other docs use a single space. |
Bah. So sue me for being old-school - I learned typing from a DOS program that taught that :) But yeah, I figured there'd be formatting stuff I'd have to change. The code snippets probably need to change spacing from four spaces to two spaces as well, or something like that.
Hmm. Per-page, or for the section as a whole? Any suggestions? |
😄
I guess the exact format of the code snippets is a bit up in the air until our new lint rules (airbnb?) are decided on.
Line 74 of Basic Reducer Structure is an example to consider. IMO the line in bold is a key idea in the "Basic State Shape" section, but it comes 20+ lines into the section. To be fair, it is in bold, but it would be a shame if people missed an important idea because they are skipping sections that start off seeming familiar. I actually hadn't considered an "as a whole" summary, but that might be a good idea. |
Okay, seems reasonable. I'll have to do some thinking about how to summarize things. |
I've now read all the sections (and also gone back and read most of the Here are some things I have been thinking about the overall structure. I am far from a technical writing expert, so feel free to tell me I am talking nonsense at any point!
I think there is a lot of cross over between "Basic Reducer Usage" stuff and the existing Reducer doc. I would be in favour of augmenting the existing Reducer doc where it is lacking and remove the "Basic Reducer Usage" material from the new docs. There is also some cross-over between "combineReducers and Beyond" and "Reducer Composition"? Some re-organization might make sense there (or it might not make sense to treat them as distinct topics at all).
01: Basic Reducer Structure There is a large overlap between this and the Reducer doc. I would be in favour 02: Splitting Reducer Logic Remove "business function" and "case function" from the glossary (they aren't 03: Functional Decomposition Drop the explanations of Merge "Splitting Reducer Logic" and "Functional Decomposition" into a single "Reducer Composition" doc. 04: Using combineReducers Remove the basic information about 05: Beyond combineReducers LGTM. Merge "Using combineReducers" with "Beyond combineReducers". Maybe "combineReducers and Beyond"? 06: Normalizing State Shape LGTM 07: Managing Normalized Data I'm not sure about the Redux-ORM stuff here. It looks neat, but doesn't feel essential to me. Removing it reduces the doc size considerably. Merge with "Normalizing State Shape" 08 - Reusing Reducer Logic LGTM Merge with Splitting Reducer Logic and Functional Decomposition 09 - Immutable Update Patterns This is very important stuff, Redux won't work as advertised if you get this 10 - Initializing State LGTM 00: Pre-Requisites: I would be in favour of moving the pre-requisites to the relevant introductions, and drop this as a stand-alone document. I also have a couple of wording suggestions and things, but I think this is |
@ellbee : Thanks, this is some great feedback! I'll have to take some time to digest it and think through things. Also hoping to get review-ish comments from a couple more people. Much appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot here.
I know you've collected a lot of information about Redux best practices and have a good encyclopedia of information about it.
The challenge with that is that it's tempting to include it all here.
I can imagine similar sections talking about actions/action creators, async actions, selectors, etc., and I wonder how much would be too much for the core documentation. The longer it gets, the less likely people will read it.
The information here is really valuable, though, and having a distilled version of all of the information that's out there is a good idea.
Would it be worth splitting some of this into a separate online book about Redux in the real-world? Or is it OK to have this as part of the core documentation? I'm not sure I know the answer, but I think it's worth talking about.
A few other comments that didn't fit anywhere specific:
- It feels like most of the community has standardized on using the Flux Standard Action specification. Is it worth mentioning that and then re-working the examples to comply?
# Basic Reducer Structure and State Shape | ||
|
||
## Basic Reducer Structure | ||
First and foremost, it's important to understand that your entire application really only has **one single reducer function**: the function that you've passed into `createStore` as the first argument. That one single reducer function ultimately needs to do several things: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really important point, and you've made it very clear here 👍
## Basic Reducer Structure | ||
First and foremost, it's important to understand that your entire application really only has **one single reducer function**: the function that you've passed into `createStore` as the first argument. That one single reducer function ultimately needs to do several things: | ||
|
||
- If the incoming `state` value is undefined, it probably needs to return some default state value to initialize the overall state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about re-working this point a bit and the first code example below.
IMO, there shouldn't be any difference in how the reducer works the first time it is called (with undefined
as the previous state) and any other time. The first code example is written to return the initial state without even looking at the action the first time. The second example, using an ES6 default parameter solves this problem.
Since Redux does call the reducer with undefined
and an initialization action before anything else, this approach works. But it doesn't match my mental model of how a reducer should be written.
How about something like this for this bullet:
"The first time the reducer is called, the state
value will be undefined
. The reducer needs to handle this case by supplying a default state value before handling the incoming action."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Good point, that should probably be tweaked a bit.
```js | ||
function counter(state, action) { | ||
if (typeof state === 'undefined') { | ||
return 0; // If state is undefined, return the initial application state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, this early return means that the first action is completely ignored. Maybe this should change so that the rest of the if
statements are called with the initial state in this case? I'm not sure how I'd write that, because I've always just used ES6 default argument syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, especially if using combineReducers
, the first call will be a randomized "probing" action designed to initialize the state, so it won't actually wind up handling anything specifically. But yeah, we should probably do state = 0
instead.
} | ||
``` | ||
|
||
This is the basic structure that a typical Redux reducer function uses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like the narrative progression here, and the refactoring of the basic reducer structure, I wonder if it would be better to have only the latter code example. That way, a hurried reader doesn't just grab the first example and use it without reading on. If there's only one code snippet, that's what people will grab.
The advantage of the first snippet is that it doesn't use ES6 features, but I'm not what our target is, so that may or may not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs certainly use ES6 just about everywhere, but given that this is intended as a "tutorial" / "how-to" section, the progression here is definitely deliberate. I would hope that someone reading this would be looking more to read and learn than "grab a snippet and go".
@@ -0,0 +1,27 @@ | |||
# Splitting Up Reducer Logic | |||
|
|||
Obviously, for any meaningful application, putting *all* your update logic into a single reducer function is quickly going to become unmaintainable. While there's no single rule for how long a function should be, it's generally agreed that functions should be relatively short and ideally only do one specific thing. Because of this, the natural response for any programmer looking at a very large chunk of code is to try to break that code into smaller pieces that are easier to understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could omit "Obviously" here. That word tends to belittle people for whom this is not yet obvious. Words like "obviously", "just", and "simply" are all things to watch out for. I'm working to remove them from my writing, so I tend to notice them more now.
Oddly enough, the use of just
in the very next section is a place where I think it's an ok use :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's definitely a bit of a nervous tic in my writing. Obviously, I should try to eliminate that :)
```js | ||
// Same as the "manual" rootReducer above | ||
const rootReducer = reduceReducers(combinedReducers, crossSliceReducer); | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce-reducers
is a nice library, and I've used it to good effect. However, you have to make sure that the first reducer in the list initializes the entire shape of the state tree, because none of the subsequent reducers will see the undefined
initial state.
It might be worth including that warning here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point.
comment : ".....", | ||
}, | ||
}, | ||
allIds : ["comment1", "comment2", "comment3", "commment4", "comment5"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to have allIds
here and below for users
? Since these are formerly-nested objects, they're all referred to by their parent objects already. Maybe I haven't read far enough yet, though :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - the idea is that for every entity type, you'd have both the lookup table and the list of all items.
} | ||
``` | ||
|
||
Behaviors like "Look up all books by this author" then become an O(n) operation, with a linear scan of the join table. Given the typical amounts of data in a client application and the speed of Javascript engines, an O(n) operation is likely to have sufficiently fast performance for most use cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's a good idea to use big-O notation here, because I'm not sure how much of the audience would understand it without further explanation. It's probably OK to leave it at "linear scan" and people would understand what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Maybe link to Wikipedia on this or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an option as well. I'm trying to put myself in the shoes of someone just learning this stuff. There are already a lot of new ideas and concepts they're trying to wrap their heads around, and the introduction of big-O notation here might be yet another one.
function commentsById(state = {}, action) { | ||
default : { | ||
if(action.entities && action.entities.comments) { | ||
return merge({}, state, action.entities.comments.byId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you continue to use Object.assign
here instead of introducing Lodash's merge
function? One less concept/dependency for the reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I borrowed this example directly from one of Dan's comments in an issue thread. The idea is that you're not replacing the existing state, you're updating it. Not sure if there's a better way to illustrate that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't Object.assign
do exactly the same thing? There might be some subtle differences that I'm not aware of. But again, trying to minimize the number of new concepts that people have to wrap their heads around at one time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they're definitely different. Object.assign
/ _.extend
are shallow, _.merge
is deep. See http://stackoverflow.com/questions/19965844/lodash-difference-between-extend-assign-and-merge .
@@ -0,0 +1,118 @@ | |||
# Initializing State |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the separate section I suggested above. I think it would be good to unify the Initializing State
stuff from the combineReducers
section into this section and eliminate it from combineReducers
. There might not be much unification to do - I think you covered most or all of the information in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth looking at. I definitely wrote the combineReducers
section well before I pulled over Dan's explanation from SO. Could certainly cross-link from "combineReducers
" to "Initializing State"
Wow. Well, I did ask for feedback :) Thanks, @randycoulman ! I'll go through and reply to a few of your comments now. Definitely sounds like I'm going to have to step back and rethink some of the overall approach, although I'm not exactly sure that that will involve. |
Some very worthwhile discussion. I would say that reducers are more of a core topic than the others. While use of action creators / thunks / selectors is certainly "idiomatic Redux", it's entirely possible to write an app without those. On the other hand, every Redux app will have reducers.
Well, there certainly are a few of those out there already. However, I think there is a lot of value in having at least a good chunk of this in the docs. Having the stamp of "officialness" is kind of a big deal - see the reaction to Create-React-App as an example. It's also more easily visible than a random blog post somewhere. It's certainly possible at least some of this could get moved elsewhere. As much as I like the Redux-ORM bit in "Managing Normalized Data", that might be a good candidate. |
Probably won't be able to start addressing this stuff until next weekend (or later). Thanks for all the comments so far. Would also appreciate feedback from @reactjs/redux , @aweary , @naw , @Phoenixmatrix , @tommikaikkonen , @mxstbr, and anyone else who's looked at this. |
@markerikson Sorry if my review was too detailed or too overwhelming. I'm not suggesting that you need to re-work the whole thing, and I hope you didn't take it that way. You've got a lot of great stuff here, and I think it will help a lot of people. And remember, I'm just one guy, so balance what I say against what others think. |
No no, this is very much the kind of feedback I was hoping to get. And yeah, I'd definitely like to hear from a few more people as well. Gotta get to bed, but I'll reply to a couple of the latest comments tomorrow. Thanks for all the feedback so far! |
Relevant suggestions: https://github.com/thejameskyle/documentation-handbook |
@markerikson I know you wanted to make some changes, but I think this is pretty good as-is. And I think this is too important to leave hanging in a PR for too long! It can always be better, but we'll be accepting typo fixes until the end of time on all the docs 😄 If you want to make any other final changes, feel free. Regardless, I'll merge this in on Sunday at some point. We can make future changes via subsequent PRs. Seriously, amazing work on this stuff! |
Heh. Thanks for the encouragement :) The main thing I need to do atm is actually make sure it builds properly with the rest of the docs. I deliberately didn't worry about that yet, only focusing on the content. For example, I'm not sure about the numbered naming of the Markdown files versus the generated HTML filenames. I may need to rename the Markdown files to produce better HTML output. Currently working on setting up a new gadget. I'll tackle this on Sunday, and let you know when I'm done. Please ping me before merging. Thanks! |
All gitbook cares about is how it's referenced in the ToC. It builds the book off of what's linked there and that's it. |
Adjust size of "Prerequisite Concepts" link Clarify wording in "Mutation" note Fix behavior in basic ES5 counter reducer example Clarify use of top-level object Add description of domain state, app state, and UI state Remove emphasis on "business" and "utility" functions Rework "Refactoring Reducers" example to cut down on repetition Remove duplication between "Using combineReducers" and "Initializing State" Remove uses of "obviously" Add note on use of reduceReducers Replace "O(n)" and "linear scan" with "a single loop" Clarify use of Lodash merge Emphasize need to update all levels of nesting Highlight common mistakes in updating
Updated the content per review comments. Now time to do some checks for Gitbook results. |
BTW, this is what you need to add to the docs/README.md to make it show up in the gitbook build: * [Structuring Reducers](/docs/recipes/StructuringReducers.md)
* [Basic Reducer Structure](/docs/recipes/reducers/01-BasicReducerStructure.md)
* [Splitting Reducer Logic](/docs/recipes/reducers/02-SplittingReducerLogic.md)
* [Refactoring Reducers Example](/docs/recipes/reducers/03-RefactoringReducersExample.md)
* [Using `combineReducers`](/docs/recipes/reducers/04-UsingCombineReducers.md)
* [Beyond `combineReducers`](/docs/recipes/reducers/05-BeyondCombineReducers.md)
* [Normalizing State Shape](/docs/recipes/reducers/06-NormalizingStateShape.md)
* [Updating Normalized Data](/docs/recipes/reducers/07-UpdatingNormalizedData.md)
* [Reusing Reducer Logic](/docs/recipes/reducers/08-ReusingReducerLogic.md)
* [Immutable Update Patterns](/docs/recipes/reducers/09-ImmutableUpdatePatterns.md)
* [Initializing State](/docs/recipes/reducers/10-InitializingState.md) I can put that at the top of the Recipes section. I think that looks best. |
Right, that was going to be my next step once I got Gitbook building okay in my repo. Unfortunately, it looks like the current Gitbook setup symlinks from |
BTW, I'm going to merge this in now and we can do some other PRs to get them into the docs site. Plus, if anyone wants to suggest further changes, they can do so against the docs themselves 👍 Great job, @markerikson! |
@markerikson For that gitbook issue, just temporarily copy the docs/README.md contents into SUMMARY.md to get it building. Whenever you have it how you'd like, just copy the changes (if any) back and make a commit. |
Possible typo in "Using combineReducers":
should be
? |
@mg : Yeah, looks like I've got a couple typos in there. Will fix those. |
And fixed. |
* Add an initial empty "Structuring Reducers" recipe. * Add initial intro text * Add "Basic Reducer Structure" section * Add initial "Basic State Shape" and "Splitting Reducer Logic" sections Note that a _lot_ of this content duplicates the existing "Reducers" doc page, which bothers me, but we'll work that out later. * Define terms and various function concepts * Add reducer refactoring examples * Add unfinished "Immutable Data" topic, reducer terms, and mutation note * Split "Structuring Reducers" into multiple pages * Add Prerequisite Concepts content * Really emphasize the prerequisites * Update StructuringReducers.md * Add example state shape and note about refactoring example * Add additional prerequisite links * Expand reducer refactoring example * Add more prerequisite links, and tweak example identifiers * Rework reducer terminology * Initial draft of "Using combineReducers" * Tweak comments for key naming * Add a bit more explanation of object key naming * Add initial draft for "Beyond combineReducers" * Tweak wording in a few spots * More prereq links, and another note on combineReducers * First draft of "Normalizing State Shape" * Add additional notes on normalization usage * Tweak wording and fix typos * Clarify reduceReducers example * Tweak wording * Fixes a typo * Add "Reusing Reducer Logic" page * Add "Immutable Update Patterns" page * Fix wrong invocation of toggleTodo which should be editTodo * Add "Managing Normalized Data" page * Tweak phrasing in "Managing Normalizing Data" * Add "Initializing State" * Tweak section headings * Address review comments for "Structuring Reducers" Adjust size of "Prerequisite Concepts" link Clarify wording in "Mutation" note Fix behavior in basic ES5 counter reducer example Clarify use of top-level object Add description of domain state, app state, and UI state Remove emphasis on "business" and "utility" functions Rework "Refactoring Reducers" example to cut down on repetition Remove duplication between "Using combineReducers" and "Initializing State" Remove uses of "obviously" Add note on use of reduceReducers Replace "O(n)" and "linear scan" with "a single loop" Clarify use of Lodash merge Emphasize need to update all levels of nesting Highlight common mistakes in updating
Initial content review for #1784 . Please give feedback on content, formatting, structure, and ordering of topics. Current content is a first draft, and I expect I'll need to make numerous changes before it's ready to merge.