Update Style Guide on Hooks #16323
Update Style Guide on Hooks #16323
Conversation
marcaaron
changed the title
| }; | ||
| ``` | ||
|
|
||
| * Avoid public methods on components and calling them via refs |
There was a problem hiding this comment.
This defensive comment is largely irrelevant if we are removing class components. The way to do this with hooks would be useImperativeHandle(), but it's kind of a rare edge case that we would reach for it at all. And generally much harder to abuse this without knowing that it's an option.
| } | ||
| ``` | ||
|
|
||
| ## Binding methods |
There was a problem hiding this comment.
This is class specific and totally unnecessary in function components.
There was a problem hiding this comment.
yay, stoked that we won't have to bind so much everywhere.
However, maybe some guidelines on when a function should be declared inside the component vs outside would be helpful?
// Outside
function doSomething() {
console.log('Hello world');
}
const MyComponent = () => {
// Inside
const doSomethingElse = () => {
console.log('Goodnight, moon');
}
return (
<View>
<Button onPress={doSomething}>
Hello world
</Button>
<Button onPress={doSomethingElse}>
Goodnight moon
</Button>
</View>
);
};There was a problem hiding this comment.
Yeah, I agree. So, this is kind of an interesting question because we have the jsx-no-bind rule enabled which will warn if you have any function in a function component that is not using useCallback(). But I also don't think that there is always a good reason to use useCallback() (even if your function has dependencies sometimes it might make sense to redeclare the function?).
There was a problem hiding this comment.
I think I would come back and update this later as part of some evolving best practices. My instinct is to say something like...
If you aren't going to memoize the function with useCallback() then make it a pure function and move it outside the component if you can. If you need to create a closure around the props or state or something else then either allow it to be redeclared or just useCallback(). Probably the community has some idea about what would be best.
|
|
||
| *_1. Stateless components: Used when you don't need to maintain state or use lifecycle methods._* | ||
|
|
||
| In many cases, we create components that do not need to have a state, lifecycle hooks or any internal variables. In other words just a simple component that takes props and renders something presentational. But often times we write them as class based components which come with a lot of cruft (for e.g., it has a state, lifecycle hooks and it is a javascript class which means that React creates instances of it) that is unnecessary in many cases. |
There was a problem hiding this comment.
This section is irrelevant because function components now can have "state" so they are not "stateless"
| ... | ||
|
|
||
| ``` | ||
| *_2. Pure components: Use to improve performance where a component does not need to be rendered too often._* |
There was a problem hiding this comment.
This whole section is a bit out of date though I suppose we can mention something about performance. React.memo() is basically the replacement for any PureComponent or shouldComponentUpdate().
|
|
||
| Let's say you have two separate components that render two different things, but you want them both to be based on the same state. You can pass a `render` property to a component or pass a function as a child to accomplish this. You can read more about how this works and when to use it in the [React Docs](https://reactjs.org/docs/render-props.html). | ||
|
|
||
| ## Do not use inheritance for other React components |
There was a problem hiding this comment.
There isn't any concern here since we won't have any class components. Hooks are designed to facilitate code reuse so while there may still be HOCs render props are going to be a less attractive pattern than hooks.
| } | ||
| ``` | ||
|
|
||
| ## isMounted is an Antipattern |
There was a problem hiding this comment.
The cleanup method of useEffect() should solve any issues with this section.
…e do not need to memoize a function?
marcaaron
changed the title
melvin-bot
bot
requested review from
danieldoglas and
sobitneupane
and removed request for
a team
|
@sobitneupane @danieldoglas One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Overall nothing here looks concerning, the changes looks pretty good. It's exciting how much we can remove from our style guide. Seems consistent with our hope that switching to hooks will reduce "gotchas" for NewDot contributors.
| } | ||
| ``` | ||
|
|
||
| ## Binding methods |
There was a problem hiding this comment.
yay, stoked that we won't have to bind so much everywhere.
However, maybe some guidelines on when a function should be declared inside the component vs outside would be helpful?
// Outside
function doSomething() {
console.log('Hello world');
}
const MyComponent = () => {
// Inside
const doSomethingElse = () => {
console.log('Goodnight, moon');
}
return (
<View>
<Button onPress={doSomething}>
Hello world
</Button>
<Button onPress={doSomethingElse}>
Goodnight moon
</Button>
</View>
);
};
There was a problem hiding this comment.
I noticed there are a couple items on the PR checklist:
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
Should we remove these? Or not yet?
Edit: Maybe we shouldn't remove those until we don't have any more class components.
| shouldShow={_.isEmpty(this.props.policy)} | ||
| }, [props.reports, props.policy]); | ||
|
|
||
| const policy = props.policy; |
There was a problem hiding this comment.
Since you're creating a local variable for policy, let's move it up and use it on line 72 where we're currently using props.policy?
Reviewer Checklist
Screenshots/VideosWebWeb.movMobile Web - ChromeAndroidmWeb.movMobile Web - SafariRPReplay_Final1680093923.MP4Desktopdesktop.moviOSiOS.movAndroidAndroid.mov |
|
Just noting that I found a bug but was able to reproduce it on production. Reported here. Also added some offline test steps. |
|
Going to go ahead and merge this. We can always adjust the style guide more if needed. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.2.92-0 🚀
|
| propsRef.current.onExit(); | ||
| }, | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
We don't need to use useRef here.
useEffect(() => {}, []) - empty array dependency - this is equivalent to componentDidMount.
So openLink is called only once regardless of props.token change.
There was a problem hiding this comment.
interesting. but does it cause any harm? if we use the props.token directly then the exhaustive-deps linter would complain about a missing dependency.
There was a problem hiding this comment.
if it complains, we can disable with a comment instead of using ref: https://expensify.slack.com/archives/C01GTK53T8Q/p1680256754764159
| const PlaidLink = (props) => { | ||
| // We are stashing initial props in a ref since we don't want a new token to trigger the link to open again | ||
| // and just want openLink() to be called once | ||
| const propsRef = useRef(props.token); |
There was a problem hiding this comment.
cc @marcaaron I think this should hold all the props not only the token
const propsRef = useRef(props);There was a problem hiding this comment.
Oh, yes, I see my mistake now
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.92-2 🚀
|
Details
Fixed Issues
$ #16325
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android