create placeholderID generator

[Judahmeek]

Let me know if I need to change the "placeholderID" to something less generic.

---

This change is 

[justin808]

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

---

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

const todos = handleActions({
  [actionTypes.ADD_TODO]: ($$state: State, { payload }: addTodoPayload) => $$state.mergeIn(
    // eslint-disable-next-line no-plusplus

remove lint statement

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 8 at r1 (raw file):

import todosActionTypes from '../actions/todos/actionTypes';
import * as todosActions from '../actions/todos';
import placeholderIDGenerator from '../services/placeholderIDGeneretor';

I like using lower case d in ID

@robwise?

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 12 at r1 (raw file):

export function* addTodo({ payload }: stringPayload): Generator<any, putEffect, any> {
  const placeholderID = yield call(placeholderIDGenerator);

why is this a generator function?

why not just a unique SHA?

---

todo-app-production/client/app/bundles/todosIndex/services/placeholderIDGeneretor.js, line 1 at r1 (raw file):

export default function* () {

why is this a generator function?

---

Comments from Reviewable

[justin808]

some comments

---

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[Judahmeek]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

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

Previously, justin808 (Justin Gordon) wrote…

remove lint statement

Will do.

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 12 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why is this a generator function? why not just a unique SHA?

Correct me if I'm wrong, but You seem to be assuming that we would only need a single placeholderID. I went with the assumption that the number of possible placeholders could be unrestricted, like starting the app with Wifi and then turning Wifi off and continuing to use the app.

---

Comments from Reviewable

[robwise]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 8 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I like using lower case d in ID @robwise?

yep, agreed

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 12 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Correct me if I'm wrong, but You seem to be assuming that we would only need a single placeholderID. I went with the assumption that the number of possible placeholders could be unrestricted, like starting the app with Wifi and then turning Wifi off and continuing to use the app.

You don't need Saga for this one, just use a normal action called createTodo that kicks off the Todo process:

const addTodo = (todo, placeholderId = `TEMP_TODO_${_.uuid}`) => ({ type: 'TODOS_CREATE', payload: _.merge(todo, { id: placeholderId }));

Now your sagas can listen for that action and then do their thing.

Using a default, you will be able to test it since you can enforce the UUID to be the same and thereby make the function pure when testing. Using string interpolation with a prefix like that is guaranteed to have a unique id, as opposed to what you were doing before which would stop working after 100 todos.

Note that this is going to make the assumption that you are using only numbers as keys in your map not true. I would consider making a separate reducer for temporary todos versus persisted Ids, that way it's very clear what is what.

---

todo-app-production/client/app/bundles/todosIndex/services/placeholderIDGeneretor.js, line 1 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why is this a generator function?

We can get rid of this function, see above

---

Comments from Reviewable

[Judahmeek]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 8 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

yep, agreed

I'll keep that in mind for future reference.

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 12 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

You don't need Saga for this one, just use a normal action called `createTodo` that kicks off the Todo process: ```js const addTodo = (todo, placeholderId = `TEMP_TODO_${_.uuid}`) => ({ type: 'TODOS_CREATE', payload: _.merge(todo, { id: placeholderId })); ``` Now your sagas can listen for that action and then do their thing. Using a default, you will be able to test it since you can enforce the UUID to be the same and thereby make the function pure when testing. Using string interpolation with a prefix like that is guaranteed to have a unique id, as opposed to what you were doing before which would stop working after 100 todos. Note that this is going to make the assumption that you are using only numbers as keys in your map not true. I would consider making a separate reducer for temporary todos versus persisted Ids, that way it's very clear what is what.

Fair point on both enforcing reproducibility and creating a separate reducer.

---

todo-app-production/client/app/bundles/todosIndex/services/placeholderIDGeneretor.js, line 1 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

We can get rid of this function, see above

Very well. In my defense, I believe this function serves the exact same purpose as _.uuid especially when being used on the client.

---

Comments from Reviewable

[robwise]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

todo-app-production/client/app/bundles/todosIndex/services/placeholderIDGeneretor.js, line 1 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Very well. In my defense, I believe this function serves the exact same purpose as `_.uuid` especially when being used on the client.

I know brutha, it was cool, just figured we ought not to reinvent wheel here

---

Comments from Reviewable

[Judahmeek]

Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions.

---

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

const actionTypes = mirrorCreator([
  'ADD_TODO',

What is the file structure convention for shared action types like ADD_TODO here?

---

Comments from Reviewable

[robwise]

Reviewed 1 of 12 files at r2. Review status: 1 of 10 files reviewed at latest revision, 5 unresolved discussions.

---

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

Previously, Judahmeek (Judah Meek) wrote…

What is the file structure convention for shared action types like `ADD_TODO` here?

This is why I was saying we need to decouple actions from the view and from the reducers. If this is true, then whether or not something is shared by a component or a reducer is irrelevant. I would put all todos actions in a todos actions folder.

Also, we should move away from mirrorCreator and just manually create them and export with named constants, that way we don't have to import actions we don't need and we can also use destructuring imports. It's also nicer to work with them since you can just use simplified names. For instance:

https://github.com/shakacode/friendsandguests/blob/master/client/app/bundles/guest-lists/constants/pendingInvitationsPageConstants/fetchPendingInvitations.js https://github.com/shakacode/friendsandguests/blob/master/client/app/bundles/guest-lists/reducers/pendingInvitationsPageReducer/isFetchingPendingInvitationsReducer.js

---

Comments from Reviewable

[Judahmeek]

Review status: 1 of 10 files reviewed at latest revision, 5 unresolved discussions.

---

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

Previously, robwise (Rob Wise) wrote…

This is why I was saying we need to decouple actions from the view and from the reducers. If this is true, then whether or not something is shared by a component or a reducer is irrelevant. I would put all todos actions in a `todos` actions folder. Also, we should move away from mirrorCreator and just manually create them and export with named constants, that way we don't have to import actions we don't need and we can also use destructuring imports. It's also nicer to work with them since you can just use simplified names. For instance: https://github.com/shakacode/friendsandguests/blob/master/client/app/bundles/guest-lists/constants/pendingInvitationsPageConstants/fetchPendingInvitations.js https://github.com/shakacode/friendsandguests/blob/master/client/app/bundles/guest-lists/reducers/pendingInvitationsPageReducer/isFetchingPendingInvitationsReducer.js

Fine by me. I only included mirrorCreator at @justin808's insistence. I prefer not to make those changes in this PR, however. Any other issues I need to address before merging this?

---

Comments from Reviewable

[robwise]

---

Reviewed 1 of 6 files at r3. Review status: 2 of 10 files reviewed at latest revision, 5 unresolved discussions.

---

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

Previously, Judahmeek (Judah Meek) wrote…

Fine by me. I only included mirrorCreator at @justin808's insistence. I prefer not to make those changes in this PR, however. Any other issues I need to address before merging this?

Roger that, yeah not sure why he wants you to do that, we moved away from that almost a year ago.

I think we can merge yeah, can you click the done button next to the easy ones you are sure you've addressed (don't do it for ones where you need the change to be re-reviewed, just simple ones);

---

Comments from Reviewable

[justin808]

Reviewed 6 of 12 files at r2, 6 of 6 files at r3. Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

todo-app-production/client/package.json, line 68 at r3 (raw file):

    "jquery-ujs": "^1.1.0-1",
    "lodash": "^4.17.4",
    "lodash-uuid": "^0.0.3",

are we still using the lodash-uuid? @judahmeek?

---

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

Previously, robwise (Rob Wise) wrote…

Roger that, yeah not sure why he wants you to do that, we moved away from that almost a year ago. I think we can merge yeah, can you click the `done` button next to the easy ones you are sure you've addressed (don't do it for ones where you need the change to be re-reviewed, just simple ones);

some misunderstanding I think...I'm fine with whatever Rob says is the current way.

---

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

export const addTodoFailure = createAction(actionTypes.ADD_TODO_FAILURE);
export const removeTodo = createAction(actionTypes.REMOVE_TODO);
export const removeTodoSuccess = createAction(actionTypes.REMOVE_TODO_SUCCESS);

why a Success one here but not for add?

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

// @flow

The file name of tempTodos => should this really be the formTodo?

Why the name temp?

---

Comments from Reviewable

[justin808]

I added a few comments.

---

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[robwise]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

todo-app-production/client/package.json, line 68 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

are we still using the lodash-uuid? @judahmeek?

We're using it, but _.uuid is part of the normal lodash package so we don't need to include it again so let's remove it.

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

The file name of `tempTodos` => should this really be the `formTodo`? Why the name temp?

Because he is using a temporary ID, I assume because he's planning on doing optimistic updating

---

Comments from Reviewable

[Judahmeek]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

todo-app-production/client/package.json, line 68 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

We're using it, but `_.uuid` is part of the normal `lodash` package so we don't need to include it again so let's remove it.

_.uuid is not part of the lodash package according to https://lodash.com/docs/4.17.4

---

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

Previously, justin808 (Justin Gordon) wrote…

why a `Success` one here but not for `add`?

Because removeTodo is to trigger the removeTodo saga and removeTodoSuccess is for async success, while addTempTodo triggers addTodo saga and addTodo only runs on async success.

If you'd like, I can add "-Success" to addTodo to make things clearer.

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

Because he is using a temporary ID, I assume because he's planning on doing optimistic updating

That's the general idea. To be honest, I wonder if it isn't overcomplicating things and we should just stick to the redux example's data flow structure.

---

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

Previously, Judahmeek (Judah Meek) wrote…

Will do.

Done.

---

todo-app-production/client/app/bundles/todosIndex/sagas/index.js, line 8 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

I'll keep that in mind for future reference.

Done.

---

todo-app-production/client/app/bundles/todosIndex/services/placeholderIDGeneretor.js, line 1 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

I know brutha, it was cool, just figured we ought not to reinvent wheel here

Done.

---

Comments from Reviewable

[Judahmeek]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

todo-app-production/client/package.json, line 68 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

_.uuid is not part of the lodash package according to https://lodash.com/docs/4.17.4

Unless you're referring to https://lodash.com/docs/4.17.4#uniqueId?

---

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

Previously, justin808 (Justin Gordon) wrote…

some misunderstanding I think...I'm fine with whatever Rob says is the current way.

Very well. I'd prefer to address this in another PR however.

---

Comments from Reviewable

[robwise]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

todo-app-production/client/package.json, line 68 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Unless you're referring to https://lodash.com/docs/4.17.4#uniqueId?

oops sry yeah same thing, I forgot they changed it my bad

---

Comments from Reviewable

[robwise]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

That's the general idea. To be honest, I wonder if it isn't overcomplicating things and we should just stick to the redux example's data flow structure.

Can you link to the one to which you're referring?

---

Comments from Reviewable

[Judahmeek]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

Can you link to the one to which you're referring?

I was thinking about http://redux.js.org/docs/basics/ExampleTodoList.html, but upon review, I remembered that's all client logic without any server interaction. That said, what do you think about this repo: https://github.com/r-park/todo-react-redux ?

---

Comments from Reviewable

[justin808]

Review status: 7 of 10 files reviewed at latest revision, 6 unresolved discussions.

---

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

Previously, Judahmeek (Judah Meek) wrote…

Because `removeTodo` is to trigger the `removeTodo` saga and `removeTodoSuccess` is for async success, while `addTempTodo` triggers `addTodo` saga and `addTodo` only runs on async success. If you'd like, I can add "-Success" to `addTodo` to make things clearer.

Yes, that's more consistent...

The addTempTodo ==> did you consider just naming as addTodo or addFormTodo? The temp todo sounds like a todo that won't be used later...

@robwise How do we do this for F&G?

---

Comments from Reviewable

[robwise]

Review status: 7 of 10 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

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

Previously, justin808 (Justin Gordon) wrote…

Yes, that's more consistent... The addTempTodo ==> did you consider just naming as addTodo or addFormTodo? The temp todo sounds like a todo that won't be used later... @robwise How do we do this for F&G?

Yeah, I would just make it addTodo

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

I was thinking about http://redux.js.org/docs/basics/ExampleTodoList.html, but upon review, I remembered that's all client logic without any server interaction. That said, what do you think about this repo: https://github.com/r-park/todo-react-redux ?

That might be kind of difficult to use as a baseline because it's very tightly integrated with Firebase architecture

---

Comments from Reviewable

[robwise]

Reviewed 1 of 12 files at r2, 1 of 6 files at r3, 16 of 17 files at r4. Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

todo-app-production/client/app/bundles/todosIndex/reducers/index.js, line 24 at r4 (raw file):

};

export default combineReducers({ todos, tempTodos, addTodoForm });

this file looks great

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.js, line 1 at r3 (raw file):

Previously, robwise (Rob Wise) wrote…

That might be kind of difficult to use as a baseline because it's very tightly integrated with Firebase architecture

Although we could rip the styles.

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.test.js, line 23 at r4 (raw file):

  const payload = { tempTodo: { id: todoId } };
  const state = tempTodosInitialState.set(todoId,
                                          $$Map({ description: 'todo', completed: true }));

I would include the ids in the actual todo since that's more realistic. Can you grab the normalizeArrayToMap utility function from F&G and use it here? Then we can do:

const state = tempTodosInitialState.merge(normalizeArrayToMap({ id: todoId, description: 'todo', completed: true }));

Also, the indentation is a bit strange here. I would really highly suggest using the prettier-eslint Atom package and turning on Format on Save that way we don't have to address in PRs

---

todo-app-production/client/app/bundles/todosIndex/sagas/sagas.test.js, line 21 at r4 (raw file):

  const result = { response: { data: 'data' } };
  nextGen = generator.next(result);
  expect(nextGen.value).toEqual(put(todosActions.addTodoSuccess({ Todo: result.response.data, tempTodo: payload })));

why capitalize Todo? It's an instance/object not a class

---

Comments from Reviewable

[Judahmeek]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

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

Previously, robwise (Rob Wise) wrote…

Yeah, I would just make it `addTodo`

Done.

---

todo-app-production/client/app/bundles/todosIndex/reducers/tempTodosReducer.test.js, line 23 at r4 (raw file):

Previously, robwise (Rob Wise) wrote…

I would include the ids in the actual todo since that's more realistic. Can you grab the `normalizeArrayToMap` utility function from F&G and use it here? Then we can do: ```js const state = tempTodosInitialState.merge(normalizeArrayToMap({ id: todoId, description: 'todo', completed: true })); ``` Also, the indentation is a bit strange here. I would really highly suggest using the `prettier-eslint` Atom package and turning on `Format on Save` that way we don't have to address in PRs

I added the package, but I can't use normalizeArrayToMap from the tempTodos because normalizeArrayToMap assumes that I want numbers for keys and tempTodos use strings.

---

todo-app-production/client/app/bundles/todosIndex/sagas/sagas.test.js, line 21 at r4 (raw file):

Previously, robwise (Rob Wise) wrote…

why capitalize `Todo`? It's an instance/object not a class

oops

---

Comments from Reviewable