-
Notifications
You must be signed in to change notification settings - Fork 73
[React] Updated some old rules #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ | |
|
||
- Only include one React component per file. | ||
- Always use JSX syntax. | ||
- Do not use `React.createElement` unless you're initializing the app from a file that is not JSX. | ||
- Do not use `React.createElement`. | ||
- Always use export default for Components. | ||
- Use `export default` for reducers, actionCreators and services. As a general rule of thumb, use `export default` for all files that have a unique object to export. | ||
|
||
|
@@ -37,23 +37,37 @@ src | |
└───app | ||
│ │ | ||
│ └───components | ||
│ │ └───baseComponents | ||
│ │ └───Input | ||
│ │ └───Text | ||
│ │ └───Button | ||
│ │ └───atoms # Can't be broken down into smaller things | ||
│ │ | └───Input | ||
│ │ | └───Text | ||
│ │ | └───Button | ||
│ │ | └───etc | ||
│ │ └───molecules # Two or more atoms | ||
│ │ | └───FormInput | ||
│ │ | └───SearchBar | ||
│ │ | └───Table | ||
│ │ | └───etc | ||
│ │ └───organisms # Two or more molecules | ||
│ │ └───LoginForm | ||
│ │ └───UserTable | ||
│ │ └───etc | ||
│ └───screens | ||
│ └───MyScreenComponent | ||
│ └───assets // Screen specific app assets | ||
│ | components | ||
│ └───assets # Screen specific app assets | ||
│ └───reducer | ||
│ | actions.js | ||
│ | reducer.js | ||
│ | selectors.js | ||
│ | effects.js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
│ └───components # Screen specific components | ||
│ | constants.js | ||
│ | i18n.js | ||
│ | index.js | ||
│ | layout.js | ||
│ | styles.scss | ||
│ | utils.js | ||
│ | ||
└───assets // General app assets | ||
└───assets # General app assets | ||
└───config | ||
| api.js | ||
| i18n.js | ||
|
@@ -64,22 +78,27 @@ src | |
│ | actions.js | ||
│ | reducer.js | ||
│ | selectors.js | ||
│ | effects.js | ||
│ | ||
└───propTypes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of proptypes we should add a folder for reusable interfaces or types. |
||
│ | Model1.js | ||
│ │ Model2.js | ||
│ | ||
└───scss | ||
└───services | ||
| MyService.js | ||
│ │ serializers.js | ||
│ └───Model | ||
│ | ModelService.js | ||
│ | serializers.js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All |
||
│ | ||
└───utils | ||
│ index.js | ||
``` | ||
|
||
## Class vs `React.createClass` vs stateless | ||
|
||
- If you have internal state and/or refs, prefer `class extends Component` over `React.createClass`. eslint: [`react/prefer-es6-class`](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prefer-es6-class.md) [`react/prefer-stateless-function`](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prefer-stateless-function.md) | ||
- Avoid `React.createClass`. eslint: [`react/prefer-es6-class`](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prefer-es6-class.md) [`react/prefer-stateless-function`](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prefer-stateless-function.md) | ||
- Prefer normal functions (not arrow functions) over classes: | ||
|
||
```jsx | ||
// bad | ||
|
@@ -91,49 +110,47 @@ src | |
}); | ||
|
||
// good | ||
class Listing extends Component { | ||
// ... | ||
render() { | ||
return <div>{this.state.hello}</div>; | ||
} | ||
function Listing({ hello }) { | ||
return <div>{hello}</div>; | ||
} | ||
``` | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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 |
||
|
||
```jsx | ||
// bad | ||
class Listing extends Component { | ||
state = { foo: 1 }; | ||
|
||
render() { | ||
return <div>{this.props.hello}</div>; | ||
return <div>{this.state.hello}</div>; | ||
} | ||
} | ||
|
||
// bad (relying on function name inference is discouraged) | ||
const Listing = ({ hello }) => ( | ||
<div>{hello}</div> | ||
); | ||
// bad | ||
const Listing = ({ hello }) => { | ||
const [foo, setFoo] = useState(1); | ||
return <div>{hello}</div>; | ||
} | ||
|
||
// good | ||
function Listing({ hello }) { | ||
const [foo, setFoo] = useState(1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a good idea 👍 |
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :agite: |
||
|
||
```jsx | ||
// bad | ||
function TextContainer extends Component { | ||
function TextContainer({ text }) { | ||
renderText = text => <span>text</span>; | ||
|
||
render() { | ||
return ( | ||
<div> | ||
{this.renderText('aText')} | ||
</div> | ||
) | ||
} | ||
return ( | ||
<div> | ||
{this.renderText(text)} | ||
</div> | ||
) | ||
} | ||
|
||
// good | ||
|
@@ -159,38 +176,38 @@ src | |
## Naming | ||
|
||
- **Extensions**: Use `.js` extension for React components. | ||
- **Filename**: For component filenames and services use PascalCase. E.g., `ReservationCard.js`. | ||
- **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) | ||
- **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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of adding here Adding this we can show 2 good practices instead of 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like it, @alejobh he means it as a "bad" example |
||
|
||
// good | ||
import ReservationCard from './ReservationCard'; | ||
import MyService from './MyService'; # webpack infers index.js | ||
import MyComponent from './MyComponent'; # webpack infers index.js | ||
|
||
// bad | ||
const ReservationItem = <ReservationCard />; | ||
const MyComponent = <MyComponent />; | ||
|
||
// good | ||
const reservationItem = <ReservationCard />; | ||
const myComponent = <MyComponent />; | ||
``` | ||
- **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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd change this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my point of view, the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 inclined to keep the styleguide as short as possible, that's why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
|
||
```jsx | ||
// MyComponent/index.js | ||
import MyComponent from './layout' | ||
|
||
class MyComponentContainer extends Component { | ||
function MyComponentContainer() { | ||
// Do smart stuff | ||
|
||
render() { | ||
return <MyComponent /> | ||
} | ||
return <MyComponent /> | ||
} | ||
|
||
// MyComponent/layout.js | ||
|
@@ -605,69 +622,43 @@ src | |
} | ||
``` | ||
|
||
- Base smart components like InputContainer, ButtonContainer, etc. if it's clear that they will want to pass all its props to it's layout component: | ||
- HTML element wrappers (Button, Input, Image, etc.) | ||
|
||
```jsx | ||
import Button from './layout'; | ||
class ButtonContainer extends Component { | ||
// do something smart | ||
|
||
render() { | ||
return <Button {...this.props} /> | ||
function Input(props) { | ||
return <input {...props} /> | ||
} | ||
} | ||
``` | ||
|
||
- Spreading objects with known, explicit props. This can be particularly useful when testing React components with Mocha's beforeEach construct. | ||
|
||
```jsx | ||
function Foo { | ||
const props = { | ||
text: '', | ||
isPublished: false | ||
} | ||
|
||
return <div {...props} />; | ||
} | ||
|
||
export default Foo; | ||
``` | ||
|
||
Notes for use: | ||
Filter out unnecessary props when possible. Also, use [prop-types-exact](https://www.npmjs.com/package/prop-types-exact) to help prevent bugs. | ||
|
||
```jsx | ||
// good | ||
render() { | ||
const { irrelevantProp, ...relevantProps } = this.props; | ||
return <WrappedComponent {...relevantProps} /> | ||
} | ||
|
||
// bad | ||
render() { | ||
const { irrelevantProp, ...relevantProps } = this.props; | ||
return <WrappedComponent {...this.props} /> | ||
} | ||
``` | ||
|
||
## Refs | ||
|
||
- Always use ref callbacks. eslint: [`react/no-string-refs`](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-string-refs.md) | ||
- Always use ref [`useRef`](https://reactjs.org/docs/hooks-reference.html#useref) for function components and [`createRef`](https://reactjs.org/docs/refs-and-the-dom.html#creating-refs) for class components. | ||
|
||
> Note: Only use refs for class components. Reasons in React docs: https://reactjs.org/docs/refs-and-the-dom.html#refs-and-functional-components | ||
|
||
```jsx | ||
// bad | ||
<Foo | ||
ref="myRef" | ||
/> | ||
<Foo ref="myRef" /> | ||
|
||
// good | ||
// bad | ||
setRef = ref => this.myRef = ref; | ||
|
||
<Foo | ||
ref={this.setRef} | ||
/> | ||
<Foo ref={this.setRef} /> | ||
|
||
// good | ||
function Bar() { | ||
const myRef = useRef(null); | ||
return <Foo ref={myRef}/> | ||
} | ||
|
||
// good | ||
class Bar extends Component() { | ||
myRef = createRef(); | ||
render() { | ||
return <Foo ref={myRef}/> | ||
} | ||
} | ||
|
||
``` | ||
|
||
## Parentheses | ||
|
@@ -809,16 +800,16 @@ src | |
|
||
```jsx | ||
// bad | ||
React.createClass({ | ||
class MyComponent extends Component { | ||
_onClickSubmit() { | ||
// do stuff | ||
}, | ||
|
||
// other stuff | ||
}); | ||
}; | ||
|
||
// good | ||
class extends Component { | ||
class MyComponent extends Component { | ||
onClickSubmit() { | ||
// do stuff | ||
} | ||
|
@@ -845,15 +836,14 @@ src | |
|
||
- Ordering for `class extends Component`: | ||
|
||
1. `getDerivedStateFromProps` | ||
1. `getDerivedStateFromError` | ||
1. optional `static` methods | ||
1. `constructor` | ||
1. `getChildContext` | ||
1. `componentWillMount` | ||
1. `constructor` # Although it shouldn't be necessary | ||
1. `componentDidMount` | ||
1. `componentWillReceiveProps` | ||
1. `shouldComponentUpdate` | ||
1. `componentWillUpdate` | ||
1. `componentDidUpdate` | ||
1. `getSnapshotBeforeUpdate` | ||
1. `componentWillUnmount` | ||
1. `componentDidCatch` | ||
1. *clickHandlers or eventHandlers* like `onClickSubmit()` or `onChangeDescription()` | ||
|
@@ -866,7 +856,7 @@ src | |
1. mapDispatchToProps (if using Redux) | ||
1. export default MyComponent | ||
|
||
- How to define `propTypes`, `defaultProps`, `contextTypes`, etc... | ||
- How to define `propTypes` and `defaultProps`. | ||
|
||
```jsx | ||
import React from 'react'; | ||
|
@@ -925,20 +915,4 @@ src | |
const WrappedComponent = composedHoc(Component); | ||
``` | ||
|
||
## Translation | ||
|
||
This JSX/React style guide is also available in other languages: | ||
|
||
-  **Chinese (Simplified)**: [JasonBoy/javascript](https://github.com/JasonBoy/javascript/tree/master/react) | ||
-  **Chinese (Traditional)**: [jigsawye/javascript](https://github.com/jigsawye/javascript/tree/master/react) | ||
-  **Español**: [agrcrobles/javascript](https://github.com/agrcrobles/javascript/tree/master/react) | ||
-  **Japanese**: [mitsuruog/javascript-style-guide](https://github.com/mitsuruog/javascript-style-guide/tree/master/react) | ||
-  **Korean**: [apple77y/javascript](https://github.com/apple77y/javascript/tree/master/react) | ||
-  **Polish**: [pietraszekl/javascript](https://github.com/pietraszekl/javascript/tree/master/react) | ||
-  **Portuguese**: [ronal2do/javascript](https://github.com/ronal2do/airbnb-react-styleguide) | ||
-  **Russian**: [leonidlebedev/javascript-airbnb](https://github.com/leonidlebedev/javascript-airbnb/tree/master/react) | ||
-  **Thai**: [lvarayut/javascript-style-guide](https://github.com/lvarayut/javascript-style-guide/tree/master/react) | ||
-  **Turkish**: [alioguzhan/react-style-guide](https://github.com/alioguzhan/react-style-guide) | ||
-  **Ukrainian**: [ivanzusko/javascript](https://github.com/ivanzusko/javascript/tree/master/react) | ||
|
||
**[⬆ back to top](#table-of-contents)** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 systemThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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