TODO in React w/ Redux

Question

Please critique my TODO App in React w/ Redux:

Screenshot

// React Component
const TodoApp = ({todos, inputText, onInputChange, onAdd, onDelete}) => {
 const numbers = range(todos.length);
 const zipped = zip(numbers, todos);
 return (
 <div>
 <h1>TODO!</h1>
 <ul>{zipped.map( (z) =>
 <li key={z[0].toString()}>
 {z[1].text}
 <button onClick={() => onDelete(z[0])}>delete</button>
 </li>
 )}
 <li>
 <input type="text" value={inputText} onChange={onInputChange}/>
 <button onClick={onAdd}>add</button>
 </li>
 </ul>
 </div>
 );
};
// Redux (State)
const ADD = 'ADD';
const UPDATE_INPUT_TEXT = 'UPDATE_INPUT_TEXT';
const DELETE='DELETE';
function todo(state = {todos: [], inputText: ""}, action) {
 switch(action.type) {
 case ADD:
 return {
 todos: state.todos.concat( {
 text: state.inputText
 }),
 inputText: ''
 };
 case UPDATE_INPUT_TEXT:
 return {
 todos: state.todos,
 inputText: action.newInputText
 };
 case DELETE:
 const filtered = state.todos.filter( (e, index) => index !== action.index);
 return {
 todos: filtered,
 inputText: ''
 };
 default:
 return state;
 }
}
const store = createStore(todo);
const renderTodo = () => {
 const st = store.getState();
 ReactDOM.render(
 <TodoApp todos={st.todos}
 inputText={st.inputText}
 onInputChange={event =>
 store.dispatch({type: UPDATE_INPUT_TEXT, newInputText: event.target.value})
 }
 onAdd={() => store.dispatch({type: ADD})}
 onDelete={(index) => store.dispatch({type: DELETE, index: index})}
 />,
 document.getElementById('root')
 )
};
store.subscribe(renderTodo);
renderTodo();

• James: Questions rather general... code would appear to follow the rules to qualify as a Redux / React app. You should consider using react-redux which provides nice store connect middleware and possibly looking at something like immutable.js to ensure immutability in your store.

Answer 1

I'm not exactly sure of your environment (I'm going to assume you're using Node), and with this assumption, I want to address a few things starting with specific things, then more general:

Specific

1. You shouldn't be keeping inputText in the global store. This creates lots of store dispatches and updates which are completely unnecessary and costly. Instead, pass the input value once you want to add a todo. This would entail the use of state in TodoApp to control the input so you'd have to change to an ES6 class, but that's favorable to the unnecessary updating of the Redux store.

2. There's no need to do the two lines:

   const numbers = range(todos.length);
   const zipped = zip(numbers, todos);
   

   You can shorten your Array#map callback to this without those two lines:

   {
    todos.map(({ text }, index) =>
    <li key={index}>
    {text}
    <button onClick={() => onDelete(index)}>delete</button>
    </li>
    )
   }
   

General

1. Organization - Separate your Redux reducers, actions, etc. into different files for organization. I'm not sure if your project is currently in one file or not, but this is general advice. This app may be a minor one, but it's good practice for larger apps to keep things organized. For example, a directory structure I would use in this case looks like this:

   src/
   ├── actions/
   │ ├── actions.js
   │ └── actionCreators.js
   ├── reducers/
   │ └── todos.js
   ├── components/
   │ └── Todos.js
   └── index.js
   

   Where action.js contains all your actions, exported using ES6 modules (which I name accordingly for uniformity):

   export const DELETE_TODO = 'DELETE_TODO';
   export const ADD_TODO = 'ADD_TODO';
   

   And actionCreators.js contains all the action creators, in other words, returns an action to be dispatched. Right now, you're using object literals left and right, which isn't very consistent. It also doesn't convey a message at a glance. I would use action creators instead, for example, if you're adding a todo:

   export const addTodo = text => ({
    type: ADD_TODO,
    text
   });
   

   Then you can dispatch the action like so:

   dispatch(addTodo('foo'));
   

   Instead of like this everytime:

   dispatch({ type: ADD_TODO, text: 'foo' });
   

   This can be applied also to another action creator named deleteTodo. And in this new structure, todos.js would be a separate file for your reducer. This is good practice in preparation for larger apps, where you may have more than one reducer. Finally Todos.js is your actual React component and index.js is where you create the store and call ReactDOM.render.

2. react-redux - Don't subscribe to state like that -- sure, it works but there's a more canonical pattern to use. That's where the react-redux comes in. Redux doesn't have anything to with React itself, but integrates well with React with the react-redux package. This is because Redux emits updates (via store.subscribe) when you dispatch and action and generate new state, which react-redux can take advantage to readably connect your components to state without having to recall ReactDOM.render every time the store changes. This allows for scaling to larger applications with more components.

   So in react-redux there's the connect higher-order component that provides state to a certain child, connecting it to the Redux store. You provide it two arguments: mapStateToProps and mapDispatchToProps. What each does can be found in more detail at the GitHub page, but they essentially take values and the dispatch function from the Redux store and pass them as props to a certain component, the same as what you're doing now, except all in a neat package. But connect has to get the store from somewhere, so react-redux also provides a <Provider> component to pass down the Redux store to ancestors while not having to physically do store.getState(). This will get you this in index.js:

   import { Provider } from 'react-redux';
   import Todos from './components/Todos.js';
   import todos from './reducers/todos.js';
   const store = createStore(todos);
   ReactDOM.render(
    <Provider store={store}>
    <Todos />
    </Provider>,
    ...
   );
   

   Then in Todos.js:

   class Todos extends React.Component {
    constructor() {
    this.state = {
    inputText: ''
    };
    }
    handleChange(e) {
    this.setState({
    inputText: e.target.value
    });
    }
    render() {
    const { todos, onAdd, onDelete } = this.props;
    return (
    <div>
    <h1>TODO!</h1>
    <ul>
    {
    todos.map(({ text }, index) => (
    <li key={index}>
    {text}
    <button onClick={() => onDelete(index)}>delete</button>
    </li>
    )
    }
    <li>
    <input type="text" value={this.state.inputText} onChange={e => this.handleChange(e)}/>
    <button onClick={() => onAdd(this.state.inputText)}>add</button>
    </li>
    </ul>
    </div>
    );
    }
   }
   const mapStateToProps = ({ todos }) => ({ todos }); //provides this.props.todos
   const mapDispatchToProps = dispatch => ({
    onAdd(text) {
    dispatch(addTodo(text)); //import this from ../actions/actionCreators.js
    },
    onDelete(index) {
    dispatch(deleteTodo(index));
    }
   });
   export default connect(mapStateToProps, mapDispatchToProps)(Todos); //connects component to Redux store
   

   Finally, this will clean up the reducer in todos.js for you:

   export default function todos(state = { todos: [] }, action) {
    switch(action.type) {
    case ADD_TODO: //import from ../actions/actions.js
    return {
    todos: state.todos.concat({
    text: action.text
    })
    };
    case DELETE_TODO:
    const filtered = state.todos.filter((e, index) => index !== action.index);
    return {
    todos: filtered
    };
    default:
    return state;
    }
   }
   

• Kevin Meredith: Thank you for this helpful, detailed review, Andrew! Re: This creates lots of store dispatches and updates which are completely unnecessary and costly., is this statement controversial at all? In other words, would any experienced developers argue for keeping inputText as I have done? I'm asking to learn!
• Andrew Li: @KevinMeredith I can't say for sure, but I don't see any upside. Personally, I believe in keeping state in the narrowest scope possible, like variables in JavaScript. With your current structure, you're dispatching an action every time you input, then rerendering the component once you dispatch. This creates updates which could be removed if you manage state within the component itself. The whole reason for actions/action creators is the ability to pass in an argument (such as action.text) to update the store. There's no foreseeable benefit to having inputText in the global store.
• Kevin Meredith: Any idea why I'm getting TypeError: Cannot read property 'map' of undefined on todos.map(({ text }, index) =>? I typed out your helpful answer, but I'm getting that error. Thanks!
• Andrew Li: @KevinMeredith Have you made sure you're using mapStateToProps?
• Kevin Meredith: Yes I am as I understand - github.com/kevinmeredith/react-counter-learning/tree/master/src.