[React] Updated some old rules #183
Conversation
| return <div>{hello}</div>; | ||
| } | ||
| ``` | ||
|
|
||
| - Avoid using helper render methods when possible. Functions that return JSX elements should probably be layout components. | ||
| - Avoid using helper render methods when possible. Functions that return JSX elements should probably be components themselves. |
| } | ||
| ``` | ||
|
|
||
| And if you don't have state or refs, only for stateless components, prefer normal functions (not arrow functions) over classes: | ||
| - Prefer normal function components (not arrow functions) over `class extends Component`. Only exception is usage of `componentDidCatch` and `getDerivedStateFromError` (which have no hooks support) |
There was a problem hiding this comment.
Given that since hooks, there's no need of using class components, seems that React's direction is towards deprecating them. Also, using 2 different ways of creating components was always kind of inconsistant in my opinion.
The actual arguments for functional components now that hooks exist are well explained in this post: https://itnext.io/react-class-components-are-dead-hint-not-yet-1d0a151173b8
This doesn't mean that we should refactor existing code. This means that, in new projects, we should not be using class components any more.
What do you think? If you can find more info regarding if we should or shouldn't use class components please share it, I'm open to discuss it
frontend/react/style-guide.md
Outdated
| - **Reference Naming**: Use PascalCase for React components and camelCase for their associated elements. eslint: [`react/jsx-pascal-case`](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-pascal-case.md) | ||
|
|
||
| ```jsx | ||
| // bad | ||
| import reservationCard from './ReservationCard'; | ||
| import myService from './MyService'; | ||
| import myComponent from './MyComponent'; |
There was a problem hiding this comment.
What do you think of adding here index.js. Like this:
import myComponent from './MyComponent/index.js';
Adding this we can show 2 good practices instead of 1.
There was a problem hiding this comment.
I like it, @alejobh he means it as a "bad" example
frontend/react/style-guide.md
Outdated
| │ | actions.js | ||
| │ | reducer.js | ||
| │ | selectors.js | ||
| │ | effects.js |
There was a problem hiding this comment.
Do you think it is a good practice to have the reducer and actions in each component? Maybe i'm not seeing the advantages, but i think that having the actions and reducer here will grow very quickly.
When having 2 or more screens that need the same action in which action do we add it? Also I think this makes it much more difficult to find specific actions and/or services.
Another thing i don't think is ver nice is that in the store we will have to import screens to generate the store.
I feel that having this practice has more disadvantages than advantages.
There was a problem hiding this comment.
I thinks this is a good idea if we decide to make a reducer per screen but keep using the reducers at a top level for shared state. I wonder if it may confuse devs if we don't add a clear definition on how to separate this (screen related state/actions -> screen level, model related state/actions -> top level).
I didn't have the chance to try this approach but i know you @damfinkel did so i would give it a try 👍
There was a problem hiding this comment.
I agree with Lucas, i think that at some point we will have reference problems if we move screens or components to another place
There was a problem hiding this comment.
Yeah ok, this is one thing that I've been thinking of for a while and will be addressed in the Advanced Redux talk we'll have in some weeks. But the idea is to mainly shift the criteria for creating reducers from "models" (Product, Pizza, Car, etc) to screens (ProductList, Menu, CarDetail, etc) and only leave "model" reducers for stuff that will really be global (like User or some global configuration we fetch from API).
Having only "model" reducers may have the following issues:
- One screen changing the store may, by mistake, influence another by changing a global state (for example, if ProductList changes
productsin Product reducer and there's another screen with a different product list that uses the same field in the store) If you don't clean the store correctly you'll have issues. - "model" reducers tend to be large and difficult to refactor, especially if its a core model of the app and it's used in a lot of screens in different ways.
- Having "screen" reducers makes it easier to refactor to/from Context+useReducer which in some cases it's a better solution than redux
- There's a lot of data in model reducers that will be changed only by one screen, which makes it hard to read when debugging one particular thing. Also, knowing which screen changed some shared field like
productsmay be problematic in some cases
Having said all this, I propose we start using both (model and screen reducers), which may need a better analysis of which data will be global and which won't.
Answering your question, if two screens need the same action is because they're modifying a shared state. This may mean one of the following:
- That state should be global, not in a specific screen
- The reducer logic may be extracted as an effect and reused in each screen's reducer, we're duplicating the action but we're reusing the effect and gaining readability I think.
There was a problem hiding this comment.
Okey, i think it is a good idea. What i would recommend is to place all the reducers in the redux folder. And to keep them with the same name as the screen. I don't think importing the reducers from the screens to create the store is a good practice. However all of this can change if we start using context, in that case i completely agree of having the reducers in the screen folder. Because you will use it only there.
There was a problem hiding this comment.
wouldn't it be the same? The only difference is that you import in store.js from the screens, but aside from that unavoidable difference, shouldn't we handle the "state management" folders the same way either if it's redux or context+reducer what we're using?
There was a problem hiding this comment.
I think the main difference is that with a store you can access all the reducers, you can be in one screen and access the state of another-one. I know we are trying to avoid that, but you can still do it. Meanwhile with context you cannot access a context of another screen because the context is the father of the screen index. I think that is the main reason i think we should treat this two state management separately. Also... i don't like importing screens for their reducers to create the store.
frontend/react/style-guide.md
Outdated
| │ │ | └───etc | ||
| │ │ └───organisms # Two or more molecules | ||
| │ │ └───LoginForm | ||
| │ │ └───UserTable | ||
| │ │ └───etc |
There was a problem hiding this comment.
I'm super in favor of this practice. But i feel it will be really difficult to maintain this on a scaling app. What i mean is that if you have all the app already designed you can start dividing the components into organisms/molecules and atoms. But if you are starting an app, that you don't completely know its full extend (Like most projects in wolox) . You will have lots of uncertainty if a component will be reused or if you need to go into a full specific atom. And once the app has start growing i feel that it is quite hard to maintain this structure.
What do you think about it?
There was a problem hiding this comment.
I feel this is as scalable as the generic components folder but it's more organized so we know where the component is placed in a hierarchy system
There was a problem hiding this comment.
I'm not sure I agree. Being an atom, molecule or organism doesn't depend on how many components use them or how reusable it is. If you make a component, you should be able to realize which category it belongs to from it's dependencies (for example, a if an Input component only has an child and nothing else, its an atom, but if it has a label and an input its a molecule), and add it in the corresponding folder. If at some point you decide to change it, and that change should move it to another category, you should move the component.
There was a problem hiding this comment.
Great!! If thats the case i love this idea. I was thinking of something far more complicated. I thought that if you wanted to create an input with a label, you had to create two atoms 1 for the input, 1 for the label and then create the molecule that would be the 2 of them combined. And then if you wanted to create an organism you had to create all the atoms, then all the molecules and then combine them in the organism.
There was a problem hiding this comment.
No, it's just the place where we put the components. This change wouldn't increase the amount of components
There was a problem hiding this comment.
I think a little more explaining is needed in the docs in order to understand where to place each new component.
Should atoms always be business-agnostic? Should molecules?
Seems to me like atoms are business-agnostic super small components. Then molecules use the atoms in a way that is useful to the actual app. And organisms are a bit higher level, where you would only have organism of each kind per screen (logins, specific model tables)? Am I understanding it right?
There was a problem hiding this comment.
Yes, more or less. I think you're right, we should add more context. I'm not sure if the business-agnostic is the criteria though, I think all three of them should be though of as reusable components (or else they wouldn't be in this folder, they'd be in their screen's folder tree). Of course, it's easier to reuse a component if it's the more business-agnostic possible, but I don't think that's always true.
For example, You may have an UserData component (organism, as it probable contains some molecules and/or atoms) that obtains the user's data from the store. That's not business-agnostic, but you will be able to reuse it anywhere you want as the user is a global state. Same thing happens with any logic that's shared across the app, or in more than one screen.
tl;dr; So I think the criteria should simply be something like: Atoms are single html elements wrapped in a component (these are more theoric, 99% of the time just use the html or the Text/Input provided by RN). Molecules are groups of Atoms. Organisms are groups of molecules. Probably should add a blogpost I read about this
frontend/react/style-guide.md
Outdated
| │ | actions.js | ||
| │ | reducer.js | ||
| │ | selectors.js | ||
| │ | effects.js |
There was a problem hiding this comment.
I thinks this is a good idea if we decide to make a reducer per screen but keep using the reducers at a top level for shared state. I wonder if it may confuse devs if we don't add a clear definition on how to separate this (screen related state/actions -> screen level, model related state/actions -> top level).
I didn't have the chance to try this approach but i know you @damfinkel did so i would give it a try 👍
|
|
||
| // good | ||
| function Listing({ hello }) { | ||
| const [foo, setFoo] = useState(1); |
There was a problem hiding this comment.
I'm not sure about encouraging the use of hooks yet. In my case I tell trainees and devs who i review to have this guide handy for consulting and it may be confusing for them as not everyone knows about them and it is not even included in the training.
I'd include this later when we have a more mature standard.
There was a problem hiding this comment.
Hmm I haven't though it that yet... But I'd like that all new trainees follow this and all new projects too. So we should make sure the training does. Since we have just changed it/are changing it, I think it's a good chance to ensure this happens. @pabloccid, @pabloferro what do you think?
There was a problem hiding this comment.
If we plan to extend the use of hooks to trainings and teach other devs about them then it's ok to me.
👍
There was a problem hiding this comment.
I'm onboard on using hooks instead of classes, but i think we should have in mind to create an express training for hooks.
There was a problem hiding this comment.
I think it's a good idea 👍
frontend/react/style-guide.md
Outdated
| ``` | ||
| - **Component Hierarchy**: | ||
| - Component files should be inside folders that match the component's name. | ||
| - Use index.js as the filename of a container component. Use `Container` as the suffix of the component's name. | ||
| - Use layout.js as the filename of a layout component. | ||
| - Use layout.js as the filename of a layout component. Only do this if your component is becoming too big, it should be an exception, not a rule. |
There was a problem hiding this comment.
I'd change this ... is becoming too big ... for ... is clearly separable into smart and dumb components ...
There was a problem hiding this comment.
In my point of view, the layout.js actually in this era of hooks that i hope we use in wolox is becoming a little bit useless, i would use smart & dumb pattern only in two cases:
- The component layout is becoming too big and is a class component with many logic inside.
- You are using a effect or a
componentDidMountapi call and you need to wrap the component with a loader HOC, in this case is mandatory to uselayout.js
There was a problem hiding this comment.
I think I agree with alejo in this. Smart/Dumb, hooks or not, is being abused when it should be an exception, not a rule. It's bad to abuse it because it almost duplicates the files and it also increases the code size.
I'm not sure if use layout unless is clearly separable into smart and dumb components is a good criteria because it's too subjective. We now have a linter rule that defined max lines. If you break that rule, you ma use this. I think that's the most objective reason to use it, other that the HOC rule alejo mentioned.
I'm inclined to keep the styleguide as short as possible, that's why is becoming too big seemed the best way to put it, but if you think it's better to go in depth in the explanation or change the wording I'm open to other suggestions.
There was a problem hiding this comment.
Having a little more experience in hooks i think that the smart/dumb component will be deprecated when using them with apollo/context. When using this two combined the components will tend to have their own logic and to be small organisms with their small behavior. Until them i agree with capi in maintaining the rule to be an exception and not a common practice. I will add to this file that it is a must to divide when the eslint error: max-lines appears
There was a problem hiding this comment.
I agree with Lucas on this. I think that if a component is getting too big, the solution is to divide it into smaller components, not divide it into index and layout. Probably the components further down the line will be more about layout and the ones at the top will have more state managing-related stuff.
I don't think there's any reason to keep layout.js files around. In any case, they would be index.js files with no logic.
frontend/react/style-guide.md
Outdated
| │ │ serializers.js | ||
| │ └───Model | ||
| │ | ModelService.js | ||
| │ | serializers.js |
There was a problem hiding this comment.
All .js files should be renamed to .ts or .tsx
frontend/react/style-guide.md
Outdated
| @@ -64,22 +78,27 @@ src | |||
| │ | actions.js | |||
| │ | reducer.js | |||
| │ | selectors.js | |||
| │ | effects.js | |||
| │ | |||
| └───propTypes | |||
There was a problem hiding this comment.
Instead of proptypes we should add a folder for reusable interfaces or types.
frontend/react/style-guide.md
Outdated
| ``` | ||
| - **Component Hierarchy**: | ||
| - Component files should be inside folders that match the component's name. | ||
| - Use index.js as the filename of a container component. Use `Container` as the suffix of the component's name. | ||
| - Use layout.js as the filename of a layout component. | ||
| - Use layout.js as the filename of a layout component. Only do this if your component is becoming too big, it should be an exception, not a rule. |
There was a problem hiding this comment.
I don't think this has to be a rule. I'd extract a layout component only if it can be reused by other containers, and also why not make it another component instead of a layout file?
damfinkel
dismissed stale reviews from mcallegari10, carlosbollero, henryzarza, and jmtov
via
2f37fe6
| @@ -175,18 +159,18 @@ src | |||
|
|
|||
| ## Naming | |||
|
|
|||
| - **Extensions**: Use `.js` extension for React components. | |||
| - **Filename**: For services use PascalCase. E.g., `ReservationCard.js` or a folder with the service name and `index.js` as filename. For React components, there must be a folder in PascalCase with its name and the component file should be `index.js` (and optionally `layout.js` if you're using [Smart/Dumb components](https://medium.com/@thejasonfile/dumb-components-and-smart-components-e7b33a698d43) | |||
| - **Extensions**: Use `.ts` extension for React components. | |||
There was a problem hiding this comment.
Should be .tsx in most cases.
| - **Extensions**: Use `.js` extension for React components. | ||
| - **Filename**: For services use PascalCase. E.g., `ReservationCard.js` or a folder with the service name and `index.js` as filename. For React components, there must be a folder in PascalCase with its name and the component file should be `index.js` (and optionally `layout.js` if you're using [Smart/Dumb components](https://medium.com/@thejasonfile/dumb-components-and-smart-components-e7b33a698d43) | ||
| - **Extensions**: Use `.ts` extension for React components. | ||
| - **Filename**: For services use PascalCase. E.g., `ReservationCard.ts` or a folder with the service name and `index.tsx` as filename. For React components, there must be a folder in PascalCase with its name and the component file should be `index.tsx` |
There was a problem hiding this comment.
| - **Filename**: For services use PascalCase. E.g., `ReservationCard.ts` or a folder with the service name and `index.tsx` as filename. For React components, there must be a folder in PascalCase with its name and the component file should be `index.tsx` | |
| - **Filename**: For services use PascalCase. E.g., `ReservationCard.ts` or a folder with the service name and `index.ts` as filename. For React components, there must be a folder in PascalCase with its name and the component file should be `index.tsx` |
|
|
||
| // good | ||
| import MyService from './MyService'; # webpack infers index.js | ||
| import MyComponent from './MyComponent'; # webpack infers index.js | ||
| import MyService from './MyService'; # webpack infers index.tsx |
There was a problem hiding this comment.
| import MyService from './MyService'; # webpack infers index.tsx | |
| import MyService from './MyService'; # webpack infers index.ts |
Summary
Updated the styleguide with some changed practises. We will be adding hook style guides here as well in the near future.
Known issues / Notes
I tried to separate it into two sections, a "primary" one and one with some rules that are kind of outdated (they are not outdated per se, they just refer to some practices that we never use or to old React APIs like Mixins) but I decided to leave them were they are because the document will have the same length and nre trainees may be using those practices.
Discussion
Added this differenciation to generic components which I think makes more clear how atomic they are: http://atomicdesign.bradfrost.com/chapter-2/