replaced redux for context, changed some files to ts and added reques… #114

damfinkel · 2020-03-06T21:06:07Z

…t hook

Summary

Replaced redux for context and added contextFactory to create contexts and useSelector and useDispatch hooks
Removed Field component, as it was coupled with redux-form
Added useAsyncRequest hook to perform requests in a component
Added TS to some files and examples of usage for context and hooks
Refactored AuthenticatedRoute, replaced for RouteItem

Screenshots

…t hook

tomiir

Looooooots of questions. Nothing really to change, at least until I clear some doubts. Sorry about the extensive review, but it was an extensive PR 🤷‍♂️
Excelente work though, this seems super solid !

tomiir · 2020-03-09T15:17:36Z

generators/app/steps/CustomizableSteps/constants.js

  SCREENS_PATH,
  UTILS_PATH,
  'aws.js',
  'tsconfig.json',
  'tsconfig.paths.json',
  'scripts/deploy.js',
  'src/react-app-env.d.ts',
-  `${COMPONENTS_PATH}/Routes/components/AuthenticatedRoute.tsx`,


What happened with the authenticated route?

I changed it to RouteItem, it's more generic. That component has the potential to receive any condition for being shown or not. For instance, if you have many user types with different access levels

tomiir · 2020-03-09T15:18:44Z

generators/app/steps/CustomizableSteps/copyFiles.js

+  console.log('Deleting default CRA files...');
  deleteFiles.bind(this)(FILES_TO_DELETE);
+
+  console.log('Adding files...');


Are these on purpose?

Yes, I used them to debug but then I thought it improved the output for the user

tomiir · 2020-03-09T15:21:45Z

generators/app/templates/src/app/components/Field/index.js

@@ -1,22 +0,0 @@
-import React from 'react';


Why remove this Field component? Can't it be used with another form library? Seems quite generic to me

The only function it had was to spread the input prop, which Redux Form provides. If we're not using redux-form, it's not very useful

tomiir · 2020-03-09T15:23:27Z

generators/app/templates/src/app/components/ProviderWrapper/index.tsx

+interface Props {
+  Context: React.Context<any>;
+  reducer: React.Reducer<any, { type: any }>;
+  initialState: any;


I'm not particularly fluent in TS but shouldn't this be an object type, rather than any? Don't know if TS requires you to make the object shape explicit (which clearly we cannot know) so in that case this would be fine.

You can specify that something is an object, which is more specific than any. Although we could support having a state which is not an object I don't see why we would do that. So I agree, I'll change it 👍

tomiir · 2020-03-09T16:30:55Z

generators/app/templates/src/app/components/Routes/components/RouteItem.tsx

+  redirectTo?: string;
+}
+
+function RouteItem({ redirectTo, ...config }: Props) {


What happens with public/private routes? Are you delegating the reddirection responsability somewhere?
What I mean is, for example, what if a non-logged user tries to access a private route? We used to handle that kind of cases inside the route component, how do you propose we do that now?
I think the best way would be for this to be included in the component, or at least create a HOC/wrapper to handle these changes.

redirectTo is a function which receives a user and returns a path to redirect in case that user doesn't meet the condition to show that route. This way we could not only avoid logged users to go to the login or non-logged users to go to the home, we can check access levels or anything we'd like from the user. And with small changes (receiving something other than a user) we could also have access to some routes depending on other factors that aren't the user if we wanted to. This component now is more flexible than the previous AuthenticatedRoute.

tomiir · 2020-03-09T16:43:21Z

generators/app/templates/src/services/AnalyticsService.js

@@ -1,13 +0,0 @@
-import { createMiddleware as createAnalyticsMiddleware } from 'redux-beacon';


I don't want this service, we weren't even using it. There's a card for GA though: https://trello.com/c/xba9v0SJ/19-google-analytics-seleccionable

tomiir · 2020-03-09T16:50:43Z

generators/app/templates/src/services/utils.ts

+) => {
+  const [state, setState] = useState(initialState);
+  const [loading, setLoading] = useState(shouldExecuteFetch || false);
+  const [error, setError] = useState<Error<E> | null>(null);


why not include the null case type in the interface definition?

I'm not sure what you mean. If you want to allow devs to use and endpoint and avoid handling the error, I don't agree. In every endpoint we should have a possible error and handle it. I think this will force us to do that, which I believe it's an improvement. Error should never be null, endpoints can always fail.
The only reason that it's nullable here is that it starts as null. And I think the definition should not be null (anything could be null in some contexts, so if we make Error nullable we should make anything nullable and it wouldn't make sense)

tomiir · 2020-03-09T16:53:49Z

generators/app/templates/src/services/utils.ts

+          if (activeRequest.current === requestId) {
+            const transformedResponse = data ? transformResponse(data) : undefined;
+            setState(transformedResponse);
+            withPostSuccess?.(transformedResponse);


don't really understand what this does 🤔

I removed the request ids, if that's what you meant here

withPostSuccess allows the user to do something with the response data at the moment the request finished

transformedResponse is a function you can optionally pass to change the shape of the response

tomiir · 2020-03-09T16:55:49Z

generators/app/templates/src/services/utils.ts

+        onPostFetch: response => {
+          setLoading(false);
+          if (response.data) {
+            withPostFetch?.(transformResponse(response.data));


I don't understand how you intend to use the transformResponse. I mean, you always mutate data on success, but you re-mutate it here as well. Shouldn't there be 2 different functions here?

I don't understand what you mean. Since both onPostFetch and onSuccess are different callbacks which receive the response, I can't extract the transformed response, so I must do it in these two places separately. withPostFetch is for doing stuff after the request finished, independantly of whether it failed or not

tomiir · 2020-03-09T16:56:18Z

tsconfig.json

@@ -0,0 +1,25 @@
+{
+  "compilerOptions": {
+    "target": "es5",


I honestly don't know, but it works. This is the tsconfig file for the bootstrap project. It didn't have one and most TS files broke. So I just copied the one from the generated project (which already existed and worked, I think @pabloferro configured it) so the bootstrap doesn't show strange TS errors. We could check this afterwards

mcallegari10 · 2020-03-16T14:41:52Z

generators/app/templates/src/app/reducer.ts

+};
+
+export const globalReducer = (state: GlobalState, action: Action): GlobalState => {
+  switch (action.type) {


Weren't we going to move the switch to a function?

It's really hard to type 😢

I think I told you that in order to have strong action typing I had to use switch. We encountered issues trying to move to a reducerCreator with an object, either because of TS limitations or because we just couldn't figure it out. After a while I decided to go with this as a first approach so as to finish

mcallegari10 · 2020-03-16T14:42:36Z

generators/app/templates/src/app/components/Routes/index.tsx

+
+  return (
+    <Router>
+      <div className={styles.container}>


This is an obligatory div? If not, I think we should delete it

I didn't put this here (I just changed it to TS). Nevertheless, I think it depends on how you look at it. It is not mandatory, but it might be good to have a container for the whole app with the height set to 100vh (or whatever size it should be) so we don't have to do that in every screen's container.

mcallegari10 · 2020-03-16T14:59:33Z

generators/app/templates/src/services/utils.test.tsx

+type True = true;
+type False = false;


Why though? Why not using type boolean? 🤔

Or just true or false directly

yeah, there's kind o a bug I think in TS with this. The problem is that apisauce has the following definition:

export interface ApiErrorResponse<T> { ok: false problem: PROBLEM_CODE originalError: AxiosError data?: T status?: number headers?: {} config?: AxiosRequestConfig duration?: number } export interface ApiOkResponse<T> { ok: true problem: null originalError: null data?: T status?: number headers?: {} config?: AxiosRequestConfig duration?: number }

The issue is that, for instance if I want to have an object with type ApiOkResponse and I set true, it shows the following error:

So I had to cast it to a type with the fixed value of true.

Although I just realized I can do this:
ok: true as true

and it appears to work. I'll change it.

Here's an issue related to this, although I don't understand if they're working on it or not: microsoft/TypeScript#24413

pabloferro · 2020-03-16T15:28:12Z

generators/app/templates/src/app/components/ProviderWrapper/index.tsx

+  initialState: U;
+}
+
+const withProvider = <T extends any, U>({ Context, reducer, initialState }: Props<U>) => (


What is this extends any doing?

I'll change it to {}, I was using it to pass as prop type for the wrapped component

LucasZibell · 2020-03-16T15:07:52Z

README.md

 |[react-router](https://github.com/ReactTraining/react-router)| Routing for React.
-|[redux-beacon](https://github.com/rangle/redux-beacon)| Analytics integration for Redux.
 |[seamless-immutable](https://github.com/rtfeldman/seamless-immutable)| Immutable data structures for JavaScript.
 |[history](https://www.npmjs.com/package/history)| Manage session history anywhere JavaScript runs.
 |[i18next](https://www.i18next.com/)| An internationalization-framework written in and for JavaScript.


Just a suggestion, if we are going to remove Redux and with that redux-form. Shouldn't we add Formik? Maybe we can have it already installed as a customized library

maybe, but I won't do it in this PR. As of right now, nothing is actually using redux-form, so I'm not removing any functionality. We should think if we want to force Formik, set it as optional dependency or what do we actually want

LucasZibell · 2020-03-16T15:09:39Z

generators/app/steps/CustomizableSteps/constants.js

-  'connected-react-router@^6.0.0',
-  'react-router-dom@^4.3.1',
-  'redux-recompose@^2.1.0',
-  'redux-form@^8.0.4',


Should we add Formik?

same answer

LucasZibell · 2020-03-16T15:11:23Z

generators/app/steps/CustomizableSteps/constants.js

 module.exports.FILES = [
  CONFIG_PATH,
  CONSTANTS_PATH,
  DOCS_README_PATH,
-  REDUX_PATH,


Don't we need to change this for a global Context Folder.? Just for global values, like Auth

global context is in ~app/context and it's already being added in this same object:

module.exports.FILES = [ ... 'src/app/reducer.ts', 'src/app/context.ts', ... ]

LucasZibell · 2020-03-16T15:12:08Z

generators/app/tasks/fileCreators.js

@@ -60,7 +67,6 @@ module.exports.createJSConfig = function createJSConfig() {
      '~screens/*': ['./src/app/screens/*'],
      '~config/*': ['./src/config/*'],
      '~constants/*': ['./src/constants/*'],
-      '~redux/*': ['./src/redux/*'],


If we add a context folder we should add it here

I think the I answered this in the previous comment

LucasZibell · 2020-03-16T15:19:58Z

generators/app/templates/src/app/screens/Dashboard/screens/Home/index.js

        </a>
      </header>
    </div>
  );
 }

-export default Home;
+export default withProvider({ Context, reducer, initialState: INITIAL_STATE })(Home);


I noticed we remove documentation from some components. Should we add a little bit of this component? Just explaining what it does and when to use it

I think all the docs I removed are for components that aren't present anymore. I like the idea of adding documenttion for this component, but I rather do it in another card so we can close this. I added it to the backlog: https://trello.com/c/ficXXNHQ/241-agregar-documentacion-para-withprovider

LucasZibell · 2020-03-16T15:23:18Z

generators/app/templates/src/app/screens/Dashboard/screens/Home/index.js

+
+  useEffect(() => {
+    dispatch(actionCreators.setFoo('React'));
+  }, [dispatch]);


I know that you are just using this to set an example. But i think it is rather confusing because here you just need a setState. You really don't want to set "React" in all the context. Maybe we can think of a better example and leave this with setState. For example we could dispatch a list of technologies and use them in a children component like a button and show a warning.

I realize it's not the best example, I just wanted to be expeditive. I don't think this is very important though, given that this component will be removed 100% of the times.

LucasZibell · 2020-03-16T15:27:43Z

generators/app/templates/src/services/utils.ts

+  );
+
+  return [state, loading, error, sendRequest];
+};


I think these 2 should be on a folder call Hooks where you can create your custom hooks. I would also add it to the main src and add an access with ~

components service hooks | useRequest | index.ts | constants.ts (in case you need it) | test.ts | useAsyncRequest ....

Im saying this because we should encourage devs to create their custom hooks which not only is a good practice, it solves lots of problems of code repetition. I think with this we normalize the place to where create them and we already give them 2 examples of how to create custom hooks

I like it, I'll change it

LucasZibell · 2020-03-16T15:36:04Z

generators/app/templates/jsconfig.json

@@ -11,7 +11,6 @@
      "~screens/*": ["./src/app/screens/*"],
      "~config/*": ["./src/config/*"],
      "~constants/*": ["./src/constants/*"],
-      "~redux/*": ["./src/redux/*"],


I don't think it is a discussion for now, but should we use ~ or @ in Viajeros we had some problems using ~ as it assumed all of those roots were imported from node modules. In amex we used @ and we could avoid the internal/external package problem using the following eslint configuration.

'import/order': [ 'error', { 'newlines-between': 'always', groups: ['builtin', 'external', ['unknown', 'internal'], 'parent', 'sibling', 'index'] } ]

With this eslint can differenciate between @apollo and @components. I think it could be a good improvment, what do you think?

I think I had issues with jest for some reason when I tried it to use @. Can you add a card in here https://trello.com/b/sZVeiFgw/departamento-de-frontend so we can do this separatedly?

pabloferro

Nice work!

pabloferro · 2020-03-16T15:33:24Z

generators/app/templates/src/app/components/Routes/paths.ts

@@ -0,0 +1,5 @@
+export default {


I'd move this to the constants file since it's relevant to the routes definition.

I had to move it elsewhere because it was generating circular dependencies. Routes constant import components and those components sometimes import paths. If those paths are in Routes, that's a circular dependency. Given that components shouldn't import routes, only path, I think it might be actually better this way.

pabloferro · 2020-03-16T15:36:31Z

generators/app/templates/src/app/components/Routes/index.tsx

+import RouteItem from './components/RouteItem';
+import styles from './styles.module.scss';
+
+function AppRoutes() {


Can you rename the component to match the folder name?

pabloferro · 2020-03-16T15:38:48Z

generators/app/templates/src/app/reducer.ts

+  payload: User;
+}
+
+export interface ResetUser {


Do you need to export individual action types?

probably not... at least not right now. I'll remove it

pabloferro · 2020-03-16T15:43:15Z

generators/app/templates/src/app/reducer.ts

+  resetUser: () => ({ type: ActionTypes.RESET_USER })
+};
+
+export const globalReducer = (state: GlobalState, action: Action): GlobalState => {


It's a good point.
I'm just thinking out loud, but wouldn't it make sense to have different contexts for different global data?
To avoid unnecessary updates, for example updating consumers of the user when the theme changes.

pabloferro · 2020-03-16T15:44:56Z

generators/app/templates/src/app/screens/Dashboard/screens/Home/reducer.ts

+  payload: string;
+}
+
+export interface ResetFoo {


Same as before, do you need to export these?

pabloferro · 2020-03-16T15:46:48Z

generators/app/templates/src/services/utils.test.tsx

+type True = true;
+type False = false;


Or just true or false directly

pabloferro · 2020-03-16T15:48:35Z

generators/app/templates/src/services/utils.test.tsx

+];
+
+const failureResponse = {
+  ok: false as False,


Suggested change

ok: false as False,

ok: false as const,

does that work?

ok: false as false works, I just tested it and I'll change it

pabloferro · 2020-03-16T15:48:47Z

generators/app/templates/src/services/utils.test.tsx

+};
+
+const successResponse = {
+  ok: true as True,


Suggested change

ok: true as True,

ok: true as const,

pabloferro · 2020-03-16T15:49:37Z

generators/app/templates/src/services/utils.ts

@@ -0,0 +1,131 @@
+import { useState, useEffect, useCallback } from 'react';


This looks great!

Maybe we can name the file requests or something like that instead of utils?

I'll take the suggestion from @LucasZibell and move them to a hooks folder to encourage creating custom hooks. I think I'll separate them too.

pabloferro · 2020-03-18T17:23:45Z

generators/app/templates/tsconfig.paths.json

+      "~schema/*": ["schema/*"],
+      "~app/*": ["app/*"],
+      "~hooks/*": ["./src/hooks/*"],
+      "~contexts/*": ["./src/contexts/*"]


"~hooks/*": ["app/hooks/*"], "~contexts/*": ["app/contexts/*"]

I'd move these to app

pabloferro · 2020-03-19T18:27:43Z

generators/app/templates/tsconfig.paths.json

@@ -11,8 +11,8 @@
      "~config/*": ["config/*"],
      "~schema/*": ["schema/*"],
      "~app/*": ["app/*"],
-      "~hooks/*": ["./src/hooks/*"],
-      "~contexts/*": ["./src/contexts/*"]
+      "~hooks/*": ["./app/hooks/*"],


Suggested change

"~hooks/*": ["./app/hooks/*"],

"~hooks/*": ["app/hooks/*"],

pabloferro · 2020-03-19T18:27:48Z

generators/app/templates/tsconfig.paths.json

-      "~hooks/*": ["./src/hooks/*"],
-      "~contexts/*": ["./src/contexts/*"]
+      "~hooks/*": ["./app/hooks/*"],
+      "~contexts/*": ["./app/contexts/*"]


Suggested change

"~contexts/*": ["./app/contexts/*"]

"~contexts/*": ["/app/contexts/*"]

replaced redux for context, changed some files to ts and added reques…

8fa58e0

…t hook

damfinkel assigned pabloferro, tomiir and mcallegari10 Mar 6, 2020

tomiir reviewed Mar 9, 2020

View reviewed changes

split request hook into two and added tests

87c532e

mcallegari10 reviewed Mar 16, 2020

View reviewed changes

pabloferro reviewed Mar 16, 2020

View reviewed changes

LucasZibell suggested changes Mar 16, 2020

View reviewed changes

pabloferro suggested changes Mar 16, 2020

View reviewed changes

damfinkel added 2 commits March 16, 2020 19:02

replaces global context for N global contexts and fixed PR comments

0e542d0

fixed useRequest test file suffix

b819750

LucasZibell approved these changes Mar 17, 2020

View reviewed changes

pabloferro suggested changes Mar 18, 2020

View reviewed changes

moved context and hooks to app folder and fixed paths

2c28c49

pabloferro suggested changes Mar 19, 2020

View reviewed changes

removed relative path in tsconfig for hooks and context

ff96713

pabloferro approved these changes Mar 19, 2020

View reviewed changes

mcallegari10 approved these changes Mar 19, 2020

View reviewed changes

tomiir approved these changes Mar 19, 2020

View reviewed changes

damfinkel merged commit e41f75c into development Mar 19, 2020

FrankIglesias deleted the context branch April 16, 2020 14:49

		@@ -1,13 +0,0 @@
		import { createMiddleware as createAnalyticsMiddleware } from 'redux-beacon';

		@@ -0,0 +1,131 @@
		import { useState, useEffect, useCallback } from 'react';

	"~contexts/": ["./app/contexts/"]
	"~contexts/": ["/app/contexts/"]

replaced redux for context, changed some files to ts and added reques… #114

replaced redux for context, changed some files to ts and added reques… #114

Conversation

damfinkel commented Mar 6, 2020

Summary

Screenshots

tomiir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damfinkel Mar 13, 2020 • 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

damfinkel Mar 13, 2020 • 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

damfinkel Mar 16, 2020 • 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

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

Choose a reason for hiding this comment

pabloferro left a 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

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

damfinkel Mar 13, 2020 •

edited

Loading

damfinkel Mar 13, 2020 •

edited

Loading

damfinkel Mar 16, 2020 •

edited

Loading