add app skeleton

[Judahmeek]

Definitely requires review. I used http://redux.js.org/docs/basics/ExampleTodoList.html as a template and modified it based on the new HelloWorld example structure. Still haven't added tests, flow, or any other goodies, but it's a start.

---

This change is 

[justin808]

Reviewed 21 of 21 files at r1. Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/actions/actionCreators.js, line 7 at r1 (raw file):

  return {
    type: actionTypes.addTodo,
    text

So skip the FSA for now? Flux standard actions...

---

todo-app-production/client/app/bundles/Todo/actions/actionTypes.js, line 2 at r1 (raw file):

/* eslint-disable import/prefer-default-export */

remove linter disable

---

todo-app-production/client/app/bundles/Todo/actions/actionTypes.js, line 5 at r1 (raw file):

export const addTodo = 'ADD_TODO';
export const setVisibilityFilter = 'SET_VISIBILITY_FILTER';
export const toggleTodo = 'TOGGLE_TODO';

we should use a helper...

yes over abstracted, but needed in real world projects

---

todo-app-production/client/app/bundles/Todo/components/Footer.jsx, line 1 at r1 (raw file):

import React from 'react'

each component will go in its own directory so we can put the styles as a sibling file

---

todo-app-production/client/app/bundles/Todo/components/Todo.jsx, line 19 at r1 (raw file):

  text: PropTypes.string.isRequired
}

we'll probably skip proptypes and use flowtype

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 13 at r1 (raw file):

        e.preventDefault()
        if (!input.value.trim()) {
          return

semicolons?

btw -- blank strings are falsey in JS

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r1 (raw file):

      }}>
        <input ref={node => {
          input = node

I think we should use a controlled component a form area of our Redux store.

@robwise @alexfedoseev can you point @judahmeek to an example?

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 28 at r1 (raw file):

  )
}
AddTodo = connect()(AddTodo)

we want to separate the container component and the dumb component parts

---

todo-app-production/client/app/bundles/Todo/containers/LinkContainer.jsx, line 47 at r1 (raw file):

)(Link)

export default LinkContainer

we're going to use React Router v4 for this

so none of this type of state will be in redux

---

todo-app-production/client/app/bundles/Todo/reducers/index.js, line 6 at r1 (raw file):

const reducers = combineReducers({
  todosReducer,

@robwise @judahmeek we don't normally have the keys named xxxxxReducer

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 27 at r1 (raw file):

    case 'ADD_TODO':
      return [
        ...state,

we'll be using Immutable.js

---

todo-app-production/client/app/bundles/Todo/store/index.js, line 13 at r1 (raw file):

export default createStore(
  rootReducer,
  undefined,

we'll soon have actions here, I believe.

---

Comments from Reviewable

[Judahmeek]

Review status: 14 of 23 files reviewed at latest revision, 12 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/actions/actionCreators.js, line 7 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

So skip the FSA for now? Flux standard actions...

Adding it right now.

---

todo-app-production/client/app/bundles/Todo/actions/actionTypes.js, line 2 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

remove linter disable

Done

---

todo-app-production/client/app/bundles/Todo/actions/actionTypes.js, line 5 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we should use a helper... yes over abstracted, but needed in real world projects

Adding Mirror-Creator. I only removed it because of @robwise's comments here

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 13 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

semicolons? btw -- blank strings are falsey in JS

I promise to have this thing fully linted and tested before final push.

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think we should use a controlled component a form area of our Redux store. @robwise @alexfedoseev can you point @judahmeek to an example?

Would definitely love an example. Finding a solution that wouldn't be overengineered for our simple textarea + button ain't easy.

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 28 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we want to separate the container component and the dumb component parts

I think I know what you mean considering this connect only injects dispatch and doesn't bind any props to the store. I'll work on this in the morning.

---

todo-app-production/client/app/bundles/Todo/containers/LinkContainer.jsx, line 47 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we're going to use React Router v4 for this so none of this type of state will be in redux

Do you want me to add React Router v4 in this commit or the next one?

---

todo-app-production/client/app/bundles/Todo/reducers/index.js, line 6 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@robwise @judahmeek we don't normally have the keys named `xxxxxReducer`

Gotcha

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 27 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we'll be using Immutable.js

First thing in the morning.

---

todo-app-production/client/app/bundles/Todo/store/index.js, line 13 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we'll soon have actions here, I believe.

Are you talking about the undefined initial state?

---

todo-app-production/client/app/bundles/Todo/components/Todo.jsx, line 19 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we'll probably skip proptypes and use flowtype

Will do.

---

todo-app-production/client/app/bundles/Todo/components/Footer.jsx, line 1 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

each component will go in its own directory so we can put the styles as a sibling file

Gotcha

---

Comments from Reviewable

[justin808]

This is coming along nicely!

---

Reviewed 14 of 14 files at r2. Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/actions/actionCreators.js, line 10 at r2 (raw file):

export const removeTodo = createAction(actionTypes.REMOVE_TODO);

export const toggleTodo = createAction(actionTypes.TOGGLE_TODO);

@robwise should we do this an alternate way?

export default {
  addTodo: createAction(actionTypes.ADD_TODO),
 removeTodo, createAction(actionTypes.REMOVE_TODO),

etc,

};

---

todo-app-production/client/app/bundles/Todo/components/Navlinks.jsx, line 8 at r2 (raw file):

const NavLinks = () => (
  <div>
    <NavLink exact to="/pending" activeClassName='active'>pending</NavLink>

capitalize Pending for label

---

todo-app-production/client/app/bundles/Todo/components/Todo/Todo.jsx, line 21 at r2 (raw file):

)

export default Todo

You will want to create a Todo/index.js file here that re-exports Todo.jsx

Also, did we agree that any file containing jsx should have jsx in the extension? @robwise @alexfedoseev

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Would definitely love an example. Finding a solution that wouldn't be overengineered for our simple textarea + button ain't easy.

@robwise @alexfedoseev @alleycat-at-git please provide an example for @judahmeek

---

todo-app-production/client/app/bundles/Todo/containers/LinkContainer.jsx, line 47 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Do you want me to add React Router v4 in this commit or the next one?

this commit is fine --> up to you!

---

Comments from Reviewable

[Judahmeek]

Review status: 12 of 28 files reviewed at latest revision, 12 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/components/NavLinks/index.jsx, line 8 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

capitalize Pending for label

Done.

---

todo-app-production/client/app/bundles/Todo/components/Todo/Todo.jsx, line 21 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

You will want to create a `Todo/index.js` file here that re-exports Todo.jsx Also, did we agree that any file containing jsx should have jsx in the extension? @robwise @alexfedoseev

Created some index files, but probably not enough of them.

By "containing JSX", are you including files like index.js that just import and export, but don't actually display any JSX syntax? Cause I'm pretty sure I add a JSX extension to any file displaying JSX syntax.

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@robwise @alexfedoseev @alleycat-at-git please provide an example for @judahmeek

Took a crack at it myself. Still would love an example.

---

todo-app-production/client/app/bundles/Todo/reducers/index.js, line 6 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Gotcha

While trying to figure out how to reduce the scope of state given to a reducer, I just discovered the context of your statement. Learning things crazy fast here (I have a headache... ;-;)! xD

---

todo-app-production/client/app/bundles/Todo/actions/actionCreators.js, line 10 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@robwise should we do this an alternate way? ```js export default { addTodo: createAction(actionTypes.ADD_TODO), removeTodo, createAction(actionTypes.REMOVE_TODO), etc, }; ```

I went ahead and did, but will wrapping the actionCreator functions in an object require any changes to the connect( mapStateToProps, actionCreators, )(Container); syntax?

---

todo-app-production/client/app/bundles/Todo/containers/LinkContainer.jsx, line 47 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this commit is fine --> up to you!

I guess saga is next.

---

Comments from Reviewable

[robwise]

Reviewed 9 of 23 files at r1, 6 of 16 files at r2, 21 of 21 files at r3. Review status: all files reviewed at latest revision, 27 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/actions/AddTodoForm/actionTypes.js, line 7 at r3 (raw file):

  'HANDLE_CHANGE',
  'HANDLE_SUBMIT',
]);

these are too generic, you're guaranteed namespace collisions which will cause strange errors. Remember, actions are dispatched to all reducers (assuming you're using combineReducers).

Also, I would highly recommend you make your action types from the frame of reference of what the user just did to interact with the application. Such as "ADD_TODO" or "SUBMIT_TODO_FORM" etc., not what you want the reducers to do in response. This is to keep the actions decoupled from the reducers and also to make your log/devtool output more useful.

---

todo-app-production/client/app/bundles/Todo/actions/AddTodoForm/index.js, line 8 at r3 (raw file):

  handleChange: createAction(actionTypes.HANDLE_CHANGE),
  handleSubmit: createAction(actionTypes.HANDLE_SUBMIT),
};

wrapping things unnecessarily in an object like this is sort of an anti-pattern. Prefer separate export const

---

todo-app-production/client/app/bundles/Todo/actions/TodoList/index.js, line 9 at r3 (raw file):

  removeTodo: createAction(actionTypes.REMOVE_TODO),
  toggleTodo: createAction(actionTypes.TOGGLE_TODO),
};

again, don't wrap in an object here

---

todo-app-production/client/app/bundles/Todo/components/NavLinks/index.jsx, line 8 at r3 (raw file):

const NavLinks = () => (
  <div>
    <NavLink exact to="/pending" activeClassName='active'>Pending</NavLink>

We're not using CSS Modules on RoR pro?

---

todo-app-production/client/app/bundles/Todo/components/Pure/Input.jsx, line 29 at r3 (raw file):

);

export default Input;

@justin808 why are we using Pure CSS? Did you just copy and paste this from BMDi? That was a client's request so I implemented, but definitely not something we should recommend people do.

Also this MaskedInput is something I was doing specifically for a date picker on BMDi and has no relevance for this project. Is ReactOnRails pro just a copy and paste of my work? I don't think BMDi would be very happy with us if we knew we were stealing their IP, not to mention it's demoralizing to have others take my work and claim as their own :(

---

todo-app-production/client/app/bundles/Todo/components/Todo/Todo.jsx, line 21 at r2 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Created some index files, but probably not enough of them. By "containing JSX", are you including files like `index.js` that just import and export, but don't actually display any JSX syntax? Cause I'm pretty sure I add a JSX extension to any file displaying JSX syntax.

I disagree with the Todo/index.js it's a lot of busy-work and noise for minimal benefit IMO. Either name the file index.jsx in the first place or just deal with the longer import

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Took a crack at it myself. Still would love an example.

controlling a component is page 1 of the react docs

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 1 at r3 (raw file):

import React from 'react'

this file should be called addTodoContainer

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r3 (raw file):

function mapStateToProps(state) {
  return {
    text: state.$$store.get('addTodoForm'),

we don't use $$ for immutable anymore

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 21 at r3 (raw file):

    case 'SHOW_ACTIVE':
      return todos.filter(t => !t.completed)
  }

this is a reducer handler in the abramov todos example, what is it doing in a container file?

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 24 at r3 (raw file):

}

const TodoList = ({ todos, onTodoClick }) => (

why no prop annotations?

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 34 at r3 (raw file):

    )}
  </ul>
)

what's up with multiple components in a single file? This is against the style guide.

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 50 at r3 (raw file):

}

const TodoListContainer = connect(

just inline the export default, you don't need the intermediate constant here

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 4 at r3 (raw file):

import { Map as $$Map } from 'immutable';
import { MappedTodo } from '../types';
import actionTypes from '../actions/TodoList/actionTypes';

prefer aliased imports so that you don't break things when moving this file around if you want to reorganize

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 20 at r3 (raw file):

   const oldTodo = $$state.get(payload);
   const newTodo = oldTodo.set('completed', !oldTodo.get('completed'));
   return $$state.set(payload, newTodo);

drop $$ from everything unless it's the actual Immutable.Map or Immutable.List etc. objects

---

todo-app-production/client/app/bundles/Todo/startup/App.jsx, line 18 at r3 (raw file):

  <Provider store={store(props)}>
    <Router basename="/todos">
      <div>

what does this div do?

---

todo-app-production/client/app/bundles/Todo/startup/App.jsx, line 26 at r3 (raw file):

              <TodoListContainer path={path} {...props} />
          )} />
        }

this is a strange way to do this. Just mount the components at the same path and use a router param to capture the filter value, otherwise you're mounting and unmounting components unnecessarily doing it like thsi

---

todo-app-production/client/app/bundles/Todo/store/index.js, line 13 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Are you talking about the undefined initial state?

drop the root $$store key it serves no purpose, just have its children be the root keys

---

todo-app-production/client/app/bundles/Todo/actions/actionCreators.js, line 10 at r2 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

I went ahead and did, but will wrapping the actionCreator functions in an object require any changes to the `connect( mapStateToProps, actionCreators, )(Container);` syntax?

yeah that is a bad idea for this reason and others, I'm not sure why you would suggest this Justin

---

Comments from Reviewable

[Judahmeek]

Review status: 12 of 29 files reviewed at latest revision, 27 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/actions/AddTodoForm/actionTypes.js, line 7 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

these are too generic, you're guaranteed namespace collisions which will cause strange errors. Remember, actions are dispatched to all reducers (assuming you're using `combineReducers`). Also, I would highly recommend you make your action types from the frame of reference of what the user just did to interact with the application. Such as "ADD_TODO" or "SUBMIT_TODO_FORM" etc., not what you want the reducers to do in response. This is to keep the actions decoupled from the reducers and also to make your log/devtool output more useful.

Great point. Fixing now.

---

todo-app-production/client/app/bundles/Todo/actions/AddTodoForm/index.js, line 8 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

wrapping things unnecessarily in an object like this is sort of an anti-pattern. Prefer separate `export const`

Fair enough. Will remove.

---

todo-app-production/client/app/bundles/Todo/components/NavLinks/index.jsx, line 8 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

We're not using CSS Modules on RoR pro?

We are. If I broke it somehow here, please let me know. Keep in mind that this is React Router v4's NavLink element here

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 21 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

this is a reducer handler in the abramov todos example, what is it doing in a container file?

I actually have no idea how that got there. I blame cats on my keyboard.

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 24 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

why no prop annotations?

Getting around to that. To be honest, I'm not exactly sure where to start.

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 34 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

what's up with multiple components in a single file? This is against the style guide.

Are you talking about the array of Todo's or are you talking about TodoList being defined in TodoListContainer?

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 4 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

prefer aliased imports so that you don't break things when moving this file around if you want to reorganize

Do you mean "prefer aliased imports" as opposed to importing default?

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 20 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

drop `$$` from everything unless it's the actual `Immutable.Map` or `Immutable.List` etc. objects

$$state here is Immutable. Actually, oldTodo and newTodo are as well, which may explain your confusion.

---

todo-app-production/client/app/bundles/Todo/startup/App.jsx, line 18 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

what does this div do?

According to https://github.com/ReactTraining/react-router/blob/v4/packages/react-router-dom/docs/BrowserRouter.md#children-node, BrowserRouter only accepts a single child element so I'm wrapping everything with that div.

---

todo-app-production/client/app/bundles/Todo/startup/App.jsx, line 26 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

this is a strange way to do this. Just mount the components at the same path and use a router param to capture the filter value, otherwise you're mounting and unmounting components unnecessarily doing it like thsi

I blame stackOverflow. Fixing now.

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 19 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

we don't use `$$` for immutable anymore

What do we use then?

---

todo-app-production/client/app/bundles/Todo/components/Todo/Todo.jsx, line 21 at r2 (raw file):

Previously, robwise (Rob Wise) wrote…

I disagree with the `Todo/index.js` it's a **lot** of busy-work and noise for minimal benefit IMO. Either name the file `index.jsx` in the first place or just deal with the longer import

I like the renaming idea.

---

Comments from Reviewable

[robwise]

Reviewed 1 of 22 files at r3, 24 of 24 files at r4. Review status: all files reviewed at latest revision, 34 unresolved discussions.

---

todo-app-production/client/app/bundles/Todo/actions/AddTodoForm/actionTypes.js, line 7 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Great point. Fixing now.

I would use

• TODOS___CHANGE_DRAFT_TODO
• TODOS___CREATE_TODO_FROM_DRAFT

In the tutorial he uses add because it's just local to his app, but here we're actually persisting things, so I would prefer the delete, create, update terminology for the actions

---

todo-app-production/client/app/bundles/Todo/components/NavLinks/index.jsx, line 8 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

We are. If I broke it somehow here, please let me know. Keep in mind that this is [React Router v4's NavLink element here](https://github.com/ReactTraining/react-router/blob/v4/packages/react-router-dom/docs/NavLink.md)

Commented on slack, you're using a string literal for the active class name here instead of a CSS-module class name. That's okay if you're doing it just for ease of use so you don't have to couple components to it, but just making sure that 's what's going on

---

todo-app-production/client/app/bundles/Todo/components/TodoList/index.jsx, line 17 at r4 (raw file):

        key={todo.id}
        {...todo}
        onClick={() => props.onTodoClick(todo.id)}

pass the onTodoClick action down to Todo and let Todo pass the id itself. It's Todo's concern how that action should be called by passing the id, not TodoList's.

Additionally, if you decide to pure render the Todos, you will be short-circuiting the purity by passing a new function to every single Todo everytime you change a single Todo

---

todo-app-production/client/app/bundles/Todo/containers/AddTodoFormContainer.jsx, line 4 at r4 (raw file):

import { connect } from 'react-redux';
import AddTodoForm from '../components/AddTodoForm';
import actionCreators from '../actions/AddTodoForm';

Don't organize your actions by how they are used by dumb components. This organization won't work when you want to reuse actions by different dumb components. You want your actions to be organized by how you're interacting with the state, not which component is doing it and not what the reducers should do in response. So I would make a single file called actions/TodoActions

---

todo-app-production/client/app/bundles/Todo/containers/AddTodoFormContainer.jsx, line 10 at r4 (raw file):

    text: state.get('AddTodoForm'),
  };
}

prefer fat arrows here so you can do implicit returns, much terser

const mapStateToProps = (state) => ({ text: state.get('AddTodoForm' });

Also, why is the reducer called form when it's just a text value and not a whole form?

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 24 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Getting around to that. To be honest, I'm not exactly sure where to start.

Looks like you got it

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 34 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Are you talking about the array of `Todo`'s or are you talking about `TodoList` being defined in `TodoListContainer`?

TodoList being defined in TodoListContainer, fixed now, nj

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 9 at r4 (raw file):

  return {
    todos: state.get('todos'),
}};

again, prefer fat arrow

---

todo-app-production/client/app/bundles/Todo/containers/TodoListContainer.jsx, line 15 at r4 (raw file):

    onTodoClick: (id) => {
      dispatch(toggleTodo(id))
}}};

don't do it this way, just pass the actions you want as an object:

const actions = { toggleTodo };

export default connect (mapStateToProps, actions)(...

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 4 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Do you mean "prefer aliased imports" as opposed to importing default?

I think you mean named imports, but no this is something different. You can set up webpack so you can just do

import actionTypes from 'actions/TodoList/actionTypes;

btw see comment about renaming the organization of the action types

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 20 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

`$$state` here is Immutable. Actually, `oldTodo` and `newTodo` are as well, which may explain your confusion.

yeah just drop it, we can use flow to indicate this stuff, only use $$ for when you import the Immutable classes, like $$Map, $$List, $$Set, etc.

---

todo-app-production/client/app/bundles/Todo/reducers/todosReducer.js, line 9 at r4 (raw file):

export type State = $$Map<MappedTodo>;

let tempID = 0;

this is a double no-no:

1. You want to write your reducers so that if you were to "rewind" history by reverting the state, then dispatch the same actions again, you'd get the same result. Using a let outside of the state like this means your functions are no longer pure, and you will not be getting the same output for the same input.
2. You never want to mutate anything in your reducers (for same reason as above). lets are by definition things that get mutated. I try to never ever use lets in JS except for extreme circumstances.

Instead, you want to keep any of this type of mutable state inside of your actions file, where you are allowed to have impure functions. I would also use _.uuid from lodash combined with some sort of prefix, otherwise these ids are going to collide with real Todo ids.

---

todo-app-production/client/app/bundles/Todo/sagas/index.js, line 1 at r4 (raw file):

/* eslint-disable no-console */

remove

---

todo-app-production/client/app/bundles/Todo/sagas/index.js, line 2 at r4 (raw file):

/* eslint-disable no-console */
import { call, put, fork, takeEvery } from 'redux-saga/effects';

we generally organize imports so that we have 3 groups, separated by blank lines:

1. (top) imports from node_modules
2. (middle) imports from outside of the bundle
3. (bottom) imports from inside the bundle in which the file currently resides

---

todo-app-production/client/app/bundles/Todo/sagas/index.js, line 4 at r4 (raw file):

import { call, put, fork, takeEvery } from 'redux-saga/effects';
import * as api from '../../../../api';
import AddTodoFormActionTypes from '../constants/AddTodoForm/actionTypes';

don't capitalize anything unless it's a React component or a Class

---

todo-app-production/client/app/bundles/Todo/sagas/index.js, line 27 at r4 (raw file):

function* addTodoSaga() {
  yield takeEvery(AddTodoFormActionTypes.SUBMIT_ADDTODOFORM, addTodo);

ADD_TODO_FORM

Where are these function names coming from? I like to divide up the functions by watchers and tasks:

export default function* root() {
  yield [addTodoSaga, removeTodoSaga];
}

If you end up with lots of saga files, this will keep it DRY so you don't have to keep repeating fork for every single thing, it would just be like:

export default function* root() {
  yield [...file1Watchers, ...file2Watchers];
}

---

todo-app-production/client/app/bundles/Todo/startup/App.jsx, line 18 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

According to https://github.com/ReactTraining/react-router/blob/v4/packages/react-router-dom/docs/BrowserRouter.md#children-node, BrowserRouter only accepts a single child element so I'm wrapping everything with that div.

oh okay well I think you can strike the first route that just redirects, because you can turn the other route into an IndexRoute which would do the same thing, then you can get rid of the div?

---

todo-app-production/client/app/bundles/Todo/store/index.js, line 14 at r4 (raw file):

const store = props => createStore(
  rootReducer,
  Immutable.fromJS(props),

If we do it this way for now that's fine, but usually you are going to want to compose the initial state by taking each reducer's initial state and merging in props from Rails

---

todo-app-production/client/app/bundles/Todo/containers/addTodo.jsx, line 1 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

this file should be called `addTodoContainer`

When you move files make sure to use git mv or use drag and drop or rename inside of Atom. Otherwise, it looks like you deleted this file and then created a different file with identical code, which means we lose all of the comments and blame history and stuff

---

Comments from Reviewable

[robwise]

lgtm