Migrate NewChatPage to functional component#20170
Conversation
melvin-bot
Bot
requested review from
stitesExpensify
and removed request for
a team
|
@stitesExpensify Please 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] |
| ); | ||
| const isOptionsDataReady = ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(props.personalDetails); | ||
|
|
||
| const sections = useMemo(() => { |
There was a problem hiding this comment.
I used useMemo here as it was also used here -
But I propose to remove it following the guidelines in React docs -
Optimizing with useMemo is only valuable in a few cases:
- The calculation you’re putting in useMemo is noticeably slow, and its dependencies rarely change.
- You pass it as a prop to a component wrapped in memo. You want to skip re-rendering if the value hasn’t changed. Memoization lets your component re-render only when dependencies aren’t the same.
- The value you’re passing is later used as a dependency of some Hook. For example, maybe another useMemo calculation value depends on it. Or maybe you are depending on this value from useEffect.
2 and 3 are not applicable here. For the first point, the dependencies change often in our case which would anyway recalculate the value.
If you agree then we can remove it from TaskAssigneeSelectorModal too because another issue there is that it uses props as a dependency when it only uses props.translate during calculation -
There was a problem hiding this comment.
discussion on useCallback, useMemo in slack
There was a problem hiding this comment.
@aimane-chnaif I will remove useMemo here if you agree.
There was a problem hiding this comment.
I think so. I don't see much benefit of performance improvement using useMemo here.
| // 1. searchTerm - when searchTerm changes updateOptionsWithSearchTerm is called by OptionsSelector's onChangeText prop | ||
| // 2. updateOptionsWithSearchTerm - it will change its reference upon each rerender unnecessarily | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [props.reports, props.personalDetails]); |
There was a problem hiding this comment.
Only added these two dependencies based on the code in componentDidUpdate.
thienlnam
requested review from
aimane-chnaif and
thienlnam
and removed request for
stitesExpensify
|
Reassigning due to #16239 (comment) |
| }, [props.isGroupChat, props.translate, selectedOptions, filteredRecentReports, filteredPersonalDetails, filteredUserToInvite, maxParticipantsReached]); | ||
|
|
||
| updateOptionsWithSearchTerm(searchTerm = '') { | ||
| const updateOptionsWithSearchTerm = (newSearchTerm = '') => { |
There was a problem hiding this comment.
I didn't wrap it in useCallback based on the guidelines in React docs -
Caching a function with useCallback is only valuable in a few cases:
- You pass it as a prop to a component wrapped in memo. You want to skip re-rendering if the value hasn’t changed. Memoization lets your component re-render only if dependencies changed.
- The function you’re passing is later used as a dependency of some Hook. For example, another function wrapped in useCallback depends on it, or you depend on this function from useEffect.
The first point is NA as OptionsSelector is not wrapped in memo so it will change anyway. The second point is NA since it is not a dependency of any hook.
Another reason is if we wrap it in useCallback then excludedGroupEmails has to be added as a dependency and it is not a state variable so it will refer to a new object in memory upon each rerender which would need another hook for memoization thus I think it's best not to add.
There was a problem hiding this comment.
@aimane-chnaif if I use function, eslint throws error -
There was a problem hiding this comment.
saw your comment on slack
I will update the PR and only use function keyword when the function is not a prop.
There was a problem hiding this comment.
didn't use function here as it was passed down as prop
| */ | ||
| getSections(maxParticipantsReached) { | ||
| const sections = []; | ||
| function NewChatPage(props) { |
There was a problem hiding this comment.
using function based on the discussion here
|
Merge conflicts and need to run prettier Slight bump @aimane-chnaif |
|
@thienlnam fixed both. |
|
reviewing now |
|
@aimane-chnaif friendly bump :) |
| * @param {Object} option | ||
| */ | ||
| createChat(option) { | ||
| const createChat = (option) => { |
| */ | ||
| createGroup() { | ||
| if (!this.props.isGroupChat) { | ||
| const createGroup = () => { |
There was a problem hiding this comment.
didn't use function here as it was passed down as prop
| toggleOption(option) { | ||
| this.setState((prevState) => { | ||
| const isOptionInList = _.some(prevState.selectedOptions, (selectedOption) => selectedOption.login === option.login); | ||
| const toggleOption = (option) => { |
|
Hey @aimane-chnaif, updated the PR as per your feedback. |
|
@Nikhil-Vats lint failing |
|
@aimane-chnaif fixed. |
|
I don't see any useCallback, useMemo for optimization. |
|
|
@aimane-chnaif |
|
@Nikhil-Vats please check
We can avoid
|
|
ok so as you said here that useMemo won't be very useful here. For useCallback, react docs says that -
None of these is applicable here that's why I didn't wrap it in What do you think? Should I add useMemo/useCallback? |
|
@thienlnam what is your suggestion of whether to use useCallback/useMemo or not? |
|
Let's go with useMemo - yes there are only a few cases where it makes a difference and there might not be a huge difference here, but this case applies
and so we don't want it to continually recalculate on each re-render |
|
@aimane-chnaif @thienlnam added |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.movandroid2.mov |
|
I think we can remove Here, the dependencies excluded are And define this outside of component lifecycle: |
|
@aimane-chnaif PR updated as per your feedback. |
|
✋ 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/thienlnam in version: 1.3.37-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.37-7 🚀
|
4 participants
Details
Migrate NewChatPage to functional component
Fixed Issues
$ #16239
Tests
Individual chat -
Group chat -
Offline tests
Follow the same steps as above but in offline mode.
QA Steps
Follow the same steps listed in the sections above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
web.mov
Mobile Web - Chrome
mWeb.chrome.mov
Mobile Web - Safari
mWeb.safari.mov
Desktop
desktop.mov
iOS
ios.mov
ios.offline.mov
Android
android1.mov
android2.mov