Fleshing out client-side #17

Judahmeek · 2017-02-26T03:37:40Z

This change is

Judahmeek · 2017-02-26T07:34:15Z

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion.

todo-app-production/client/app/api/index.js, line 35 at r1 (raw file):

};

export const callApi = ({ type, payload }: FSA) => {

Flow doesn't like the way I'm handling payload here:

app/api/index.js:43
 43:       return submitEntity(route.method, `${route.endpoint}${payload.id}`, payload);
                                                                         ^^ property `id`. Property cannot be accessed on possibly undefined value
 43:       return submitEntity(route.method, `${route.endpoint}${payload.id}`, payload);
                                                                 ^^^^^^^ undefined

app/api/index.js:45
 45:       return submitEntity(route.method, `${route.endpoint}${payload}`);
                                                                 ^^^^^^^ undefined. This type cannot be coerced to
 45:       return submitEntity(route.method, `${route.endpoint}${payload}`);
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ string

Should I listen to flow or shut it up?

Comments from Reviewable

robwise · 2017-02-26T09:31:59Z

Reviewed 26 of 26 files at r1.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

todo-app-production/client/.bootstraprc, line 22 at r1 (raw file):

    extractStyles: false
  test:
    extractStyles: false

Are we sure this is correct? I thought this was true in test?

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

      "^todosIndex(.*)$": "<rootDir>/app/bundles/todosIndex$1",

      "\\.(css|less)$": "<rootDir>/app/libs/interfaces/CSSModule.js"

let's drop the less and add scss

todo-app-production/client/app/api/index.js, line 24 at r1 (raw file):

 * @param {Object|number} entity - Request body to post.
 * @returns {Promise} - Result of ajax call.
 */

jsdoc shouldn't be necessary if we use flow IMO, flow makes the code self-documenting so you are guaranteed it doesn't get out of date

todo-app-production/client/app/api/index.js, line 25 at r1 (raw file):

 * @returns {Promise} - Result of ajax call.
 */
const submitEntity = (method: MethodType, endpoint: string, entity: any) => {

what if I want to upload multiple entities? Then this isn't named correctly. We should have two levels of the abstraction.

The first one only knows about http method and seeing up the CSRF token etc., as well as setting the pending_ajax_requests helper (see F&G), but needs to know nothing about redux. It's presumptuous to think the only time the app ever needs to make an API call that it's one-to-one mapped to a redux action. If that's the case, we probably didn't need to be using saga in the first place because the app has such simple requirements.

Then there is a second set of methods which are specific to each controller. Check out F&G's api folder for an example of what I mean for this second level of abstraction.

todo-app-production/client/app/api/index.js, line 35 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Flow doesn't like the way I'm handling payload here:

app/api/index.js:43
 43:       return submitEntity(route.method, `${route.endpoint}${payload.id}`, payload);
                                                                         ^^ property `id`. Property cannot be accessed on possibly undefined value
 43:       return submitEntity(route.method, `${route.endpoint}${payload.id}`, payload);
                                                                 ^^^^^^^ undefined

app/api/index.js:45
 45:       return submitEntity(route.method, `${route.endpoint}${payload}`);
                                                                 ^^^^^^^ undefined. This type cannot be coerced to
 45:       return submitEntity(route.method, `${route.endpoint}${payload}`);
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ string

Should I listen to flow or shut it up?

See above comment, we're unnecessarily coupling this implementation to redux

todo-app-production/client/app/api/routes.js, line 1 at r1 (raw file):

// @flow eslint-disable import/no-extraneous-dependencies

if you're getting errors here then it's because eslint's import resolver isn't set up correctly, this shouldn't error

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

// @flow

Sorry didn't notice this before, we need to move this to its own folder called either actionTypes/toddosActionTypes.js and not put inside of the actions folder. Same for any other action type file

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

  },
  errorsInitialState,
);

I think we should go with this style (note use of aliases and use of new keyword which we should use for clarity since we're instantiating an instance of a class using a constructor, even though it works without it):

// @flow
import { handleActions } from 'redux-actions';
import { List as $$List } from 'immutable';
 
import type { errorPayload } from 'types';
import { addTodoFailure, removeTodoFailure } from 'actions/todos/actionTypes';
 
// types
export type State = $$List<Error>;

// initial state
export const errorsInitialState = new $$List();

// helpers
const pushPayload =  (state: State, { payload }: errorPayload) => state.push(payload);

// handlers
const handlers = {
  [addTodoFailure]: pushPayload,
  [removeTodoFailure]: pushPayload,
};

// reducer
export default handleActions(handlers, errorsInitialState);

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

import { addTodoFailure, removeTodoFailure } from '../actions/todos';

try to use flow even in your tests

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

  {
    [addTodoSuccess]: (state: State, { payload }: addTodoSuccessPayload) =>
      state.merge(normalizeArrayToMap(payload.todo)),

when we implement normalize it will change this so we can just use merge, but fine for now

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

import type { putEffect, IOEffect } from 'redux-saga/effects';
import { callApi } from 'app/api';
import { addTodo as addTodoActionType, removeTodo as removeTodoActionType } from '../actions/todos/actionTypes';

need a line break above

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

  const { response, error } = yield call(callApi, action);
  if (response) {
    // But there is no data on a successful delete?

That's okay, the following is my personal opinion: Redux also can act as sort of a log of what is going on, so it's useful to see that it was successful and then maybe we respond to it maybe we don't.

todo-app-production/client/app/libs/constants/httpVerbs.js, line 4 at r1 (raw file):

export const GET = 'GET';
export const POST = 'POST';
export const PUT = 'PUT';

Can we solve whatever problem this is solving just by using flow to ensure we're using a valid http verb?

todo-app-production/client/app/libs/utils/normalizr/index.js, line 4 at r1 (raw file):

import { arrayOf as __arrayOf, normalize as __normalize } from 'normalizr';
import _ from 'lodash/fp';

I always like to keep lodash/fp at the bottom of the imports as I need to quickly ascertain whether it's in the file or not because I use it so frequently, and it provides a nice sort of end cap for the third-party libs. Sorry if this is wrong in the source code

todo-app-production/client/app/libs/utils/normalizr/index.test.js, line 1 at r1 (raw file):

import { normalizeMapIdKeys, normalizeArray, normalizeArrayToMap } from './index';

let's try and always use flow even in our tests

todo-app-production/client/app/libs/utils/normalizr/index.test.js, line 5 at r1 (raw file):

describe('libs/utils/normalizr', () => {
  test('normalizeArray', () => {
    it('creates an object with numeric ids as the keys', (): void => {

we can drop the void this was a requirement of flow 0.33 but they fixed it

todo-app-production/client/webpack-helpers/set-plugins.js, line 32 at r1 (raw file):

      Tooltip: 'exports-loader?Tooltip!bootstrap/js/dist/tooltip',
      Util: 'exports-loader?Util!bootstrap/js/dist/util',
    }),

@alexfedoseev can you double check this real fast? I'm not up to date with the latest bootstrap-loader best practices

todo-app-production/config/routes.rb, line 5 at r1 (raw file):

  get "/lists(/*others)", to: "lists#index"

  resources :todos, except: { :new, :edit }, defaults: { format: "json" }

we don't allow edit?

Comments from Reviewable

Judahmeek · 2017-02-27T22:06:58Z

Review status: 19 of 34 files reviewed at latest revision, 18 unresolved discussions.

todo-app-production/client/.bootstraprc, line 22 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

Are we sure this is correct? I thought this was true in test?

Hmmm, this entire section is commented out in F&G so I'm going to go ahead and copy that.

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

Previously, robwise (Rob Wise) wrote…

let's drop the less and add scss

Done.

todo-app-production/client/app/api/index.js, line 24 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

jsdoc shouldn't be necessary if we use flow IMO, flow makes the code self-documenting so you are guaranteed it doesn't get out of date

Flow only documents type, not purpose. Since you'll be using this code as an example while consulting, wouldn't further documentation make things easier for your clients to identify specific concepts?

todo-app-production/client/app/api/index.js, line 25 at r1 (raw file):

what if I want to upload multiple entities? Then this isn't named correctly.
I'd argue that "Entity" can refer to the request body, but I totally agree that I should have investigated F&G's api methods before reimplementing the wheel here.

todo-app-production/client/app/api/routes.js, line 1 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

if you're getting errors here then it's because eslint's import resolver isn't set up correctly, this shouldn't error

We match F&G's import resolver, which means this must be a webpack issue. I'll investigate further.

Comments from Reviewable

Judahmeek · 2017-02-27T23:47:55Z

Review status: 14 of 40 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

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

Previously, robwise (Rob Wise) wrote…

I think we should go with this style (note use of aliases and use of new keyword which we should use for clarity since we're instantiating an instance of a class using a constructor, even though it works without it):

// @flow
import { handleActions } from 'redux-actions';
import { List as $$List } from 'immutable';
 
import type { errorPayload } from 'types';
import { addTodoFailure, removeTodoFailure } from 'actions/todos/actionTypes';
 
// types
export type State = $$List<Error>;

// initial state
export const errorsInitialState = new $$List();

// helpers
const pushPayload =  (state: State, { payload }: errorPayload) => state.push(payload);

// handlers
const handlers = {
  [addTodoFailure]: pushPayload,
  [removeTodoFailure]: pushPayload,
};

// reducer
export default handleActions(handlers, errorsInitialState);

Done.

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

Previously, robwise (Rob Wise) wrote…

try to use flow even in your tests

I tried to, but get 104 errors because Jest and Flow don't get along even after updating flow-typed.

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

Previously, robwise (Rob Wise) wrote…

need a line break above

Done.

todo-app-production/client/app/libs/utils/normalizr/index.js, line 4 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

I always like to keep lodash/fp at the bottom of the imports as I need to quickly ascertain whether it's in the file or not because I use it so frequently, and it provides a nice sort of end cap for the third-party libs. Sorry if this is wrong in the source code

By "at the bottom", do you mean below all the imports or just below the other third party libraries?

todo-app-production/client/app/libs/utils/normalizr/index.test.js, line 1 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

let's try and always use flow even in our tests

Tried. Doesn't work with Jest even after updating flow-typed.

todo-app-production/client/app/libs/utils/normalizr/index.test.js, line 5 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

we can drop the void this was a requirement of flow 0.33 but they fixed it

Done.

todo-app-production/client/webpack-helpers/set-plugins.js, line 32 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

@alexfedoseev can you double check this real fast? I'm not up to date with the latest bootstrap-loader best practices

I pulled this straight from bootstrap-loader's readme so I sure hope it's up-to-date.

todo-app-production/config/routes.rb, line 5 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…

we don't allow edit?

Should we? I assumed that React would just create the edit form. Besides, if necessary, we could always pull any necessary information through a #show request, right?

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

Previously, robwise (Rob Wise) wrote…

Sorry didn't notice this before, we need to move this to its own folder called either actionTypes/toddosActionTypes.js and not put inside of the actions folder. Same for any other action type file

What's the benefit of that file structure? Less duplication of action types?

Comments from Reviewable

robwise · 2017-02-28T12:47:09Z

todo-app-production/client/app/bundles/todosIndex/sagas/sagas.test.js

@@ -25,12 +25,13 @@ describe('addTodo Saga', () => {
  it('handles async errors', () => {
    const description = 'todo description';
    const id = 'todoId';
+    const payload = { description, id };

    const action = todosActions.addTodo(description, id);
    const generator = sagas.addTodo(action);

    let nextGen = generator.next();


Try to avoid using lets. You don't need to keep reassigning the variable like this. Just do:

const gen = sagas.addTodo(action); // .... gen.next();

robwise · 2017-02-28T12:47:59Z

todo-app-production/client/app/bundles/todosIndex/sagas/index.js

@@ -1,22 +1,22 @@
 // @flow
 import { call, put, fork, takeEvery } from 'redux-saga/effects';
 import type { putEffect, IOEffect } from 'redux-saga/effects';
-import { callApi } from 'app/api';
+import * as api from 'app/api/todos';


Needs a blank line above this

robwise · 2017-02-28T12:48:21Z

todo-app-production/client/app/api/todos.js

+export const removeTodo = (todoId: number) => {
+  const url = todosScope(`/${todoId}`);
+  return apiCall.delete({ url });
+};


robwise · 2017-02-28T12:49:27Z

todo-app-production/client/app/libs/utils/index.js

+export function isObject(subject: any) {
+  return typeof subject === 'object' && Object.prototype.toString.call(subject) === '[object Object]';
+}
+


We must have written this a long time ago, I think we can just use _.isPlainObject instead?

robwise · 2017-02-28T12:51:03Z

todo-app-production/client/app/libs/utils/env/index.js

+export const isProduction = !!RAILS_ENV && RAILS_ENV === Environment.PRODUCTION;
+export const isStaging = !!RAILS_ENV && RAILS_ENV === Environment.STAGING;
+export const isProductionLike = !isDev && !isTest;
+export const isServerRendering = SERVER_RENDERING;


Where are we using these? These have never really worked right on F&G because of Heroku Preboot, so even when you're in production, it still thinks you're in staging. Are we sure we need these?

They were referenced at https://github.com/shakacode/react-on-rails-pro/pull/17/files#diff-167e965b00696dfa51189e22e267cb84, but I doubt they're absolutely necessary.

It's pretty simple code though, so I wouldn't say it doesn't work right. More like you might need an additional env_var to signal when Heroku Preboot is active?

robwise · 2017-02-28T12:51:45Z

todo-app-production/client/app/libs/utils/api/ajaxRequestTracker.js

+
+const ATTRIBUTE_NAME = 'data-are_ajax_requests_pending';
+
+let $$pendingAjaxRequestUuids = new $$Set();


I must have wrote this a long time ago, let's drop the $$ except on $$Set

Judahmeek · 2017-03-01T03:06:20Z

Review status: 8 of 122 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

todo-app-production/client/.flowconfig, line 8 at r3 (raw file):


[libs]
./flow-typed/

Craziest thing: adding ./app/libs/interfaces/ to this file (no other changes) instantly breaks Flow's acceptance of CSS Modules now.

Comments from Reviewable

robwise · 2017-03-01T11:53:24Z

todo-app-production/client/app/libs/utils/api/ajaxRequestTracker.js

-  const updateBodyAttribute = () => bodyEl.setAttribute(ATTRIBUTE_NAME, hasPendingAjaxRequests());
+  if (bodyEl == null) {
+    throw new Error('document.body is undefined!');
+  }


Why would this ever be the case?

Flow wants null checks or it gives a Method cannot be called on possibly null value error.

robwise · 2017-03-01T11:54:01Z

todo-app-production/client/app/libs/utils/api/ajaxRequestTracker.js

+  }
+
+  const hasPendingAjaxRequests = () => !pendingAjaxRequestUuids.isEmpty();
+  const updateBodyAttribute = () => bodyEl.setAttribute(ATTRIBUTE_NAME, hasPendingAjaxRequests().toString());


J/C why toSring?

Because flow demands a string for setAttribute.

robwise · 2017-03-01T11:55:54Z

todo-app-production/client/app/libs/utils/api/apiCall.js

    return this.callApi('PUT', params);
  },

-  delete(params) {
+  delete(params: any) {


these are string | {} not any

robwise · 2017-03-01T11:57:41Z

todo-app-production/client/app/libs/utils/api/apiCall.js

    if (response.ok) return response;

    return response.json().then(errData => {
-      const isBadCsrfToken = response.status === 401 && response.message === 'FnG: Bad Authenticity Token';
+      const isBadCsrfToken = response.status === 401 && errData.message === 'Bad Authenticity Token';


J/C on change from response to errData. Do we need to change this on F&G?

Probably. Flow says that response doesn't have a message parameter. What I did passes flow, but I won't be able to say that it actually works until I'm able to test it. I'm not well acquainted with Fetch API or Promises yet and the documentation isn't as clear as I would like.

robwise · 2017-03-01T11:59:41Z

todo-app-production/client/app/libs/utils/api/index.js

 import { stringify } from 'qs';
 import _ from 'lodash/fp';

 import Environment from 'app/libs/constants/Environment';
 import * as env from 'app/libs/utils/env';

-export function buildUrl(path, query) {
+export function buildUrl(path: string, query: Object) {


We should stay away from the Object flow type. This allows passing a function or class, which would break the code here:

I'll see if I can't get more specific. Suggestions would be appreciated.

The only option I'm currently aware of is using an exact option type for query: https://flowtype.org/docs/objects.html#exact-object-types

robwise · 2017-03-01T12:00:19Z

todo-app-production/client/app/libs/utils/api/index.test.js

 import { buildUrl } from './index';

 describe('libs/utils/api', () => {
-  test('{ buildUrl }', () => {
+  describe('{ buildUrl }', () => {
    it('combines a path and an object of key values into a url with a query', () => {


does the it need to become test?

No, it is an alias for test. The problem was that nested test's automatically pass without any complaint.

http://facebook.github.io/jest/docs/api.html#testname-fn

robwise · 2017-03-01T12:01:04Z

todo-app-production/client/app/libs/utils/normalizr/index.test.js

    it('creates an object with numeric ids as the keys', () => {
      const array = [{ id: 1 }];

      const actual = normalizeArray(array);
-      const expected = { 1: { id: 1 } }; // eslint-disable-line no-useless-computed-key
+      const expected = { [1]: { id: 1 } }; // eslint-disable-line no-useless-computed-key


J/C why you needed to add the brackets?

Flow again: facebook/flow#380

robwise · 2017-03-01T12:01:11Z

todo-app-production/client/app/libs/utils/normalizr/index.test.js

    it('creates a map with numeric ids as the keys', () => {
      const array = [{ id: 1 }];

      const actual = normalizeArrayToMap(array);
-      const expected = normalizeMapIdKeys({ 1: { id: 1 } }); // eslint-disable-line no-useless-computed-key,max-len
+      const expected = normalizeMapIdKeys({ [1]: { id: 1 } }); // eslint-disable-line no-useless-computed-key,max-len


Same Q as above?

robwise · 2017-03-01T12:05:05Z

todo-app-production/client/webpack-helpers/set-devtool.test.js

 const setDevtool = require('./set-devtool');

 describe('webpack-helpers/set-devtool', () => {
-  test('when builderConfig.devtool is not defined', () => {
+  describe('when builderConfig.devtool is not defined', () => {
    it('outputs to a public path', () => {


why are these all still it instead of test?

Because it is an alias for test: http://facebook.github.io/jest/docs/api.html#testname-fn

robwise · 2017-03-01T12:10:19Z

todo-app-production/client/webpack-helpers/set-entry.test.js

        const actual = setEntry(builderConfig, {}).entry.vendor;

-        expect(actual).toBe(expect.stringContaining(expected));
+        expect(actual).toEqual(expect.arrayContaining(expected));


Can't you just do

const expected = expect.arrayContaining('react-addons-perf'); const actual = setEntry(builderConfig, {}).entry.vendor; expect(actual).toEqual(expected);

String matching is for when you have a regex I think, simple equality will do here

Same goes for others below

You're right. stringMatching() is unnecessary here.

Judahmeek added 3 commits February 25, 2017 21:31

errorReducer & Jest aliases

ccc6b59

add Bootstrap 4

1a0b326

add fetch API

e219bc0

incorporate F&G callApi

472bc1a

Judahmeek added 2 commits February 27, 2017 16:36

separate actions & types

d34062d

refactor reducer code structure

3c0c68c

per review

994bff3

robwise suggested changes Feb 28, 2017

View reviewed changes

resolve flow & jest conflicts

230c1d0

Judahmeek force-pushed the flesh-out branch from df25a17 to 230c1d0 Compare March 1, 2017 03:02

add flow to utils

552bdfd

robwise suggested changes Mar 1, 2017

View reviewed changes

per review

6fce62b

Judahmeek merged commit 7441d81 into master Mar 3, 2017

Judahmeek deleted the flesh-out branch March 3, 2017 23:04


		const ATTRIBUTE_NAME = 'data-are_ajax_requests_pending';

		let $$pendingAjaxRequestUuids = new $$Set();

Fleshing out client-side #17

Fleshing out client-side #17

Conversation

Judahmeek commented Feb 26, 2017 • edited by justin808 Loading

Judahmeek commented Feb 26, 2017

robwise commented Feb 26, 2017

Judahmeek commented Feb 27, 2017

Judahmeek commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judahmeek Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judahmeek commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judahmeek Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judahmeek commented Feb 26, 2017 •

edited by justin808

Loading

Judahmeek Mar 1, 2017 •

edited

Loading

Judahmeek Mar 2, 2017 •

edited

Loading