search

shakacode / old-react-on-rails-examples

Various example apps for React on Rails, outdated
1 stars 0 forks source link

Fleshing out client-side #17

Closed Judahmeek closed 7 years ago

Judahmeek commented 7 years ago

This change is Reviewable

Judahmeek commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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): ```js // @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; // 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

Judahmeek commented 7 years ago

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