Migrate AutoUpdateTime to functional component #16968
Conversation
puneetlath
changed the title
|
@rushatgabhane @jasperhuangg 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] |
src/components/AutoUpdateTime.js
Outdated
| return () => { | ||
| clearTimeout(timerRef.current); | ||
| }; | ||
| }, [updateCurrentTime]); |
There was a problem hiding this comment.
If we want this code to be equivalent to what it was before we should maybe remove all the dependencies. Since updateCurrentTime() after every update before. Now it only runs when the reference for updateCurrentTime changes (i.e. useCallback() returns a new function because the currentUserLocalTime changed).
Putting it all together that would end up just looking like this:
useEffect(() => {
if (timerRef.current) {
clearTimeout(timerRef.current);
timerRef.current = null;
}
const millisecondsUntilNextMinute = (60 - currentUserLocalTime.seconds()) * 1000;
timerRef.current = setTimeout(() => {
setCurrentUserLocalTime(getCurrentUserLocalTime());
}, millisecondsUntilNextMinute);
return () => {
clearTimeout(timerRef.current);
};
});There was a problem hiding this comment.
That said, what you have here might work the same - but I didn't think too much about it.
There was a problem hiding this comment.
I agree that this may be an equivalent of the previous class implementation, but I believe while creating functional component we should slightly change the mindset and instead of mapping class component's functionality into functional one we should try to re-write the same functionality from scratch using new tools.
Previously the above code was called on componentDidMount and componentDidUpdate lifecycle events - we should forget about lifecycle (not completely, just keep it in mind it exists and may have an impact on our code) and think when (on what prop/state change) this effect should be called.
So in the proposed code, I would add the dependency array - maybe empty one will be enough, maybe it should depend on some props changes (excluding currentUserLocalTime - please see my other comment). But leaving it without the deps array at all will cause it to be called on every single re-render which should be avoided.
There was a problem hiding this comment.
I actually do think we want an infinite loop here. Because for as long as the user stays on this page, we want to keep updating the clock. So I think the behavior is correct where:
- Timer is set when component initially loads
- 1 min later, currentTime is updated
- This causes useEffect to be called and set another 1 min timer
- 1 min later, currentTime is updated
- repeat, every min
Can y'all double-check me that the function makes sense now?
There was a problem hiding this comment.
I think it's kind of strange actually now that I've looked at it and taking into consideration @burczu's notes. Maybe we should use a setInterval() instead to run the logic once per minute?
There was a problem hiding this comment.
We can see what @burczu thinks as well.
There was a problem hiding this comment.
Ok haha I see it now. Looks like it can work. Yeah I don't really have super strong feelings but think some kind of interval makes more sense than a timeout.
There was a problem hiding this comment.
Ach, you guys are right - using setInterval makes much more sense in this scenario! And i like the idea of extracting it into separate hook.
There was a problem hiding this comment.
I thought about it more and while I think the separate hook works, I think I prefer the other method for two reasons:
- It updates the clock at the top of the minute, rather than every 60 seconds from when the component is loaded
- It's the most simple/straightforward
If y'all strongly disagree with that, let me know. Otherwise, this is ready for re-review!
There was a problem hiding this comment.
tbh I'm fine with whatever. we kinda over-engineered it because the component is only used on the Details page. It's hard to imagine anyone is going to sit there for minutes at a time or that anyone will notice that the time is updated at the precise top of the minute.
src/components/AutoUpdateTime.js
Outdated
| return () => { | ||
| clearTimeout(timerRef.current); | ||
| }; | ||
| }, [updateCurrentTime]); |
There was a problem hiding this comment.
I agree that this may be an equivalent of the previous class implementation, but I believe while creating functional component we should slightly change the mindset and instead of mapping class component's functionality into functional one we should try to re-write the same functionality from scratch using new tools.
Previously the above code was called on componentDidMount and componentDidUpdate lifecycle events - we should forget about lifecycle (not completely, just keep it in mind it exists and may have an impact on our code) and think when (on what prop/state change) this effect should be called.
So in the proposed code, I would add the dependency array - maybe empty one will be enough, maybe it should depend on some props changes (excluding currentUserLocalTime - please see my other comment). But leaving it without the deps array at all will cause it to be called on every single re-render which should be avoided.
|
@burczu over to you for the final review and checklist. |
src/components/AutoUpdateTime.js
Outdated
| useEffect(() => { | ||
| // If the preferredLocale or timezone changes, we want to update the displayed time immediately | ||
| setCurrentUserLocalTime(getCurrentUserLocalTime()); | ||
| }, [getCurrentUserLocalTime, props.preferredLocale, props.timezone.selected]); |
There was a problem hiding this comment.
The getCurrentUserLocalTime method already depends on props.preferredLocale and props.timezone.selected so I think it's redundant here
There was a problem hiding this comment.
Yes, I noticed that too. But the linter doesn't like it when I remove it 😅. Is there any downside to having that redundant dependency?
There was a problem hiding this comment.
I mean, you should remove props.preferredLocale and props.timezone.selected here, because it is already used as dependency in the getCurrentUserLocalTime. So this way:
const getCurrentUserLocalTime = useCallback(() => (
DateUtils.getLocalMomentFromDatetime(
props.preferredLocale,
null,
props.timezone.selected,
)
), [props.preferredLocale, props.timezone.selected]);
...
useEffect(() => {
// If the preferredLocale or timezone changes, we want to update the displayed time immediately
setCurrentUserLocalTime(getCurrentUserLocalTime());
}, [getCurrentUserLocalTime]);There was a problem hiding this comment.
Ah got it! That makes sense. Updated.
There was a problem hiding this comment.
Actually... after the above change there is no sense to have two separate useEffect's cause they both have the same dependency array... so we can merge them into single one:
useEffect(() => {
// If the preferredLocale or timezone changes, we want to update the displayed time immediately
setCurrentUserLocalTime(getCurrentUserLocalTime());
// If the user leaves this page open, we want to make sure the displayed time is updated every minute when the clock changes
// To do this we create an interval to check if the minute has changed every second and update the displayed time if it has
const interval = setInterval(() => {
const currentMinute = new Date().getMinutes();
if (currentMinute !== minuteRef.current) {
setCurrentUserLocalTime(getCurrentUserLocalTime());
minuteRef.current = currentMinute;
}
}, 1000);
return () => clearInterval(interval);
}, [getCurrentUserLocalTime]);That would be the last change request from me :)
There was a problem hiding this comment.
Oh yeah, great point! Thanks a lot. Updated.
Reviewer Checklist
Screenshots/VideosWebauto-update-time-web.mp4Mobile Web - Chromeauto-update-time-web-mobile-chrome.mp4Mobile Web - Safariauto-update-time-web-mobile-safari.mp4Desktopauto-update-time-desktop.mp4iOSauto-update-time-ios.mp4Androidauto-update-time-android.mp4 |
puneetlath
dismissed stale reviews from burczu, jasperhuangg, and marcaaron
via
c3fdb16
|
✋ 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/marcaaron in version: 1.2.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.2.99-6 🚀
|
Details
This PR makes the AutoUpdateTime class component into a functional component.
Fixed Issues
$ #16117
Tests
http://localhost:8080/details?login=puneet%40expensify.com)Local timefield lists the local time with timezone and that it looks accurateOffline tests
This component works the exact same offline as online, so can be tested the exact same way.
QA Steps
staging.new.expensify.com/details?login=puneet%40expensify.com)Local timefield lists the local time with timezone and that it looks accuratePR 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