Source code is ready for review

[EricSimons]

Hey all - we're 100% source code complete and would love your feedback!

The documentation around the codebase is a little slim right now but we're working to have it done within the next few days. The codebase is pretty straightforward but if you have any questions, feel free to post them here and @vkarpov15 can answer them.

Looking forward to hearing your thoughts!

[altaywtf]

hello, source code looks helpful but I wonder is there a specific reason for not using react-redux?

[vkarpov15]

It's an extra topic to teach and I'd like to minimize the number of new libraries introduced in the course. Nothing against react-redux, it's a sweet lib

[njj]

@EricSimons The pattern used in the reducers should be different. While you are utilizing Object.assign to create a new object, you're still reassigning the state parameters.

Example: https://github.com/GoThinkster/redux-review/blob/master/src/reducers/profile.js#L6-L11

This could be easily updated by returning the result of the Object.assign, rather then reassigning state.

return Object.assign({}, state,
  /* etc.. */
)

[vkarpov15]

@njj fair point, will fix

[zhouzi]

Any reason for using require and not ES2015's import? Also, "use strict" is the default behavior of modules so babel's going to add it automatically.

Maybe some destructuration could improve readability too, e.g:

const Router = require('react-router');

// ...
<Router.Router history={Router.hashHistory}>
    <Router.Route path="/" component={App}>
        <Router.IndexRoute component={Home} />
        <Router.Route path="login" component={Login} />
        // ...
    </Router.Route>
</Router.Router>
// ...

Would become:

import { Router, Route, IndexRoute, hashHistory }  from 'react-router';

// ...
<Router history={hashHistory}>
    <Route path="/" component={App}>
        <IndexRoute component={Home} />
        <Route path="login" component={Login} />
        // ...
    </Route>
</Router>
// ...

[zhouzi]

Oh and one last thing for today: redux encourages you to provide a default value for the state within a reducer, e.g:

function todos (state = [], action) {}

Might be a good idea to add that too.

[vkarpov15]

Re: https://github.com/GoThinkster/redux-review/issues/4#issuecomment-216065037, will do.

Re: destructuring, that seems to be the standard in the react community so I think it's the right way to go.

[markerikson]

It's an extra topic to teach and I'd like to minimize the number of new libraries introduced in the course. Nothing against react-redux, it's a sweet lib

I... would argue against that. It's taught in the Redux docs, it's used by everybody who's using React... if this is supposed to be an example of how to use Redux, and it's built using React, you ought to be demonstrating best practices there as well.

[EricSimons]

That's a good point. We had originally wanted to show the plumbing of how React & Redux work separately, but now I'm thinking we should briefly explain what's going on under the hood of react-redux in the course and use it in the code. @markerikson what did you think of the codebase otherwise?

[markerikson]

Haven't had a chance to look it over yet - was just glancing at the issues list. I'll try to check it out later.

[vkarpov15]

@markerikson whatever happened to https://twitter.com/dan_abramov/status/725091443690356736 ? :) My beef is that this course already is going to have to touch on JSX, webpack, babel, react, and redux, and I'm loathe to bring in yet another concept because I don't want this course to turn into one of those ludicrous "redux is easy, all you need is these 30 packages and this 100 line webpack config file and you can make 'hello world!' appear on the screen!" blog posts. Any way we can do without it or is this a hard requirement for getting this into the redux repo?

[markerikson]

Whoa, whoa, hang on.

I may have been given commit access to Redux, but I am in no way an official arbiter of what gets into the repo. I've added a lot to the docs, which is why Dan gave me commit access, but I've stayed away from touching any PRs that have to do with the code.

But, having said that: as I understand it, the entire point of this repo is to demonstrate a full-blown, working, no-kidding, Redux-based application (per https://github.com/reactjs/redux/issues/1353). React is the most commonly used view layer for Redux, and you guys picked that.

My personal opinion is that if you're going to use React with Redux, and you're not using the React-Redux package, which is the "official bindings" between the two, it is an anti-pattern. And if that's what you're teaching, you are doing your students a disservice.

I've had to correct a number of people asking questions in Reactiflux#redux who were trying to effectively re-implement what connect() does because they didn't know it existed, or didn't understand the basic mechanics of how it worked. Helped out one guy with that just last night, actually. He thanked me profusely after I clarified how it worked and gave him a couple examples (feel free to check out the chat logs).

I still haven't actually gone through the code in this repo, but a quick search for "store" turns up sections like Login.js#L12-L49, which appears to be importing the store reference directly, initializing component state from the store's state, and manually setting up action dispatching. Honestly, I've never seen that initialization pattern used anywhere else before. And, beyond that, the FAQ specifically advises against directly importing the store. Now yes, I did write the FAQ myself, but based on existing discussions and advice, and Dan vetted the content before accepting the PR.

In contrast, I've been trying to collect some links to actual real Redux apps, and found a few more yesterday. MapStore2, Wordpress-Calypso, and Panther all appear to be very well-written applications, and all of them use connect().

So. I realize that this is a sample project you guys have put together, and that you weren't obligated to do so, and I do appreciate the work you've put into it. But, especially given that this is supposed to demonstrate a "real" application using best practices, I _strongly_ urge you to redo the UI layer to use connect(), and include that in your tutorials. I believe it will absolutely be worth the time it takes to rework the code and teach the concepts, and will be much more useful to anyone trying to learn from this example.

[vkarpov15]

Yeah I did a little more research and looks like react-redux can help clean the code up a bit. Will do, thanks for the suggestion. Any further suggestions are most welcome :)

[markerikson]

Yeah, that's sort of the point - the code will be shorter, less tightly coupled, and more testable. And I honestly don't think that React-Redux is that much to explain. mapStateToProps is pretty simple, and you can hold off on mapDispatchToProps until a bit later, and just handwave the existence of this.props.dispatch a bit.

If it helps at all, I've got a couple snippets demonstrating usage of mapState and mapDispatch at https://gist.github.com/markerikson/121c77a01c453466361a9c6434a08620 and https://gist.github.com/markerikson/f46688603e3842af0f9720dea05b1a9e.

I'll try to look over the rest of the codebase later, but not sure exactly when I'll have time.

[zhouzi]

Dan did a great job at introducing those concepts in his egghead's serie: Getting Started With Redux. He starts by showing what the code would look like without the redux/react-redux libraries and then add them to the mix, making it way clearer.

IMHO, the most complex part is the tooling so it may be a good idea to not focus on it too much.

[eliaslfox]

I know this is a bit late. But I was looking through the source code, and I was wondering why the auth token wasn't saved in redux. Storing it as part of a module is a pattern I have never seen before.

[EricSimons]

@eliaslfox I'm pretty sure @vkarpov15 did that for separation of concerns. The only time a JWT is needed is when the app is attempting to make XHRs, so he bundled it in with the custom superagent middleware (and it gets populated on page load and/or on login+register). It's not needed for any application state, so putting it in redux wasn't necessary.