Skip to content
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.

Sign up for GitHub

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

Migrate AutoUpdateTime to functional component #16968

Merged
merged 14 commits into from
Apr 11, 2023
Merged

Migrate AutoUpdateTime to functional component #16968

Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
303480b
Add functional component
puneetlath Apr 5, 2023
6fcfd98
Add state
puneetlath Apr 5, 2023
8a6e225
Migrate getCurrentUserLocalTime function
puneetlath Apr 5, 2023
8761775
Migrate getTimezoneName function
puneetlath Apr 5, 2023
d251915
Migrate updateCurrentTime function
puneetlath Apr 5, 2023
ea17bd6
Add useEffect lifecycle method
puneetlath Apr 5, 2023
81684b1
Fix lint error
puneetlath Apr 5, 2023
da57dbe
Dont call getCurrentUserLocalTime when setting initial state
puneetlath Apr 6, 2023
c58507d
Use memo instead of callback for timezone name
puneetlath Apr 6, 2023
64df919
Move updateCurrentTime functionality directly into useEffect
puneetlath Apr 6, 2023
4fd9db3
Immediately update displayed time when timezone or locale props are u…
puneetlath Apr 6, 2023
0ae7b9f
Use interval instead of timeout for udpating clock
puneetlath Apr 7, 2023
c3fdb16
Update dependency array for useEffect to remove unnecessary dependencies
puneetlath Apr 11, 2023
7fa46af
Combine useEffects
puneetlath Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 50 additions & 60 deletions src/components/AutoUpdateTime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
* The time auto-update logic is extracted to this component to avoid re-rendering a more complex component, e.g. DetailsPage.
*/
import {View} from 'react-native';
import React, {PureComponent} from 'react';
import React, {
useCallback,
useEffect,
useRef,
useState,
} from 'react';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import DateUtils from '../libs/DateUtils';
Expand All @@ -22,85 +27,70 @@ const propTypes = {
...withLocalizePropTypes,
};

class AutoUpdateTime extends PureComponent {
constructor(props) {
super(props);
this.getCurrentUserLocalTime = this.getCurrentUserLocalTime.bind(this);
this.updateCurrentTime = this.updateCurrentTime.bind(this);
this.getTimezoneName = this.getTimezoneName.bind(this);
this.state = {
currentUserLocalTime: this.getCurrentUserLocalTime(),
};
}

componentDidMount() {
this.updateCurrentTime();
}

componentDidUpdate() {
// Make sure the interval is up to date every time the component updates
this.updateCurrentTime();
}

componentWillUnmount() {
clearTimeout(this.timer);
}

function AutoUpdateTime(props) {
/**
* @returns {moment} Returns the locale moment object
*/
getCurrentUserLocalTime() {
return DateUtils.getLocalMomentFromDatetime(
this.props.preferredLocale,
const getCurrentUserLocalTime = useCallback(() => (
DateUtils.getLocalMomentFromDatetime(
props.preferredLocale,
null,
this.props.timezone.selected,
);
}
props.timezone.selected,
)
), [props.preferredLocale, props.timezone.selected]);

const [currentUserLocalTime, setCurrentUserLocalTime] = useState(getCurrentUserLocalTime());
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
const timerRef = useRef(null);

/**
* @returns {string} Returns the timezone name in string, e.g.: GMT +07
*/
getTimezoneName() {
const getTimezoneName = useCallback(() => {
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
// With non-GMT timezone, moment.zoneAbbr() will return the name of that timezone, so we can use it directly.
if (Number.isNaN(Number(this.state.currentUserLocalTime.zoneAbbr()))) {
return this.state.currentUserLocalTime.zoneAbbr();
if (Number.isNaN(Number(currentUserLocalTime.zoneAbbr()))) {
return currentUserLocalTime.zoneAbbr();
}

// With GMT timezone, moment.zoneAbbr() will return a number, so we need to display it as GMT {abbreviations} format, e.g.: GMT +07
return `GMT ${this.state.currentUserLocalTime.zoneAbbr()}`;
}
return `GMT ${currentUserLocalTime.zoneAbbr()}`;
}, [currentUserLocalTime]);

/**
* Update the user's local time at the top of every minute
*/
updateCurrentTime() {
if (this.timer) {
clearTimeout(this.timer);
this.timer = null;
const updateCurrentTime = useCallback(() => {
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
if (timerRef.current) {
clearTimeout(timerRef.current);
timerRef.current = null;
}
const millisecondsUntilNextMinute = (60 - this.state.currentUserLocalTime.seconds()) * 1000;
this.timer = setTimeout(() => {
this.setState({
currentUserLocalTime: this.getCurrentUserLocalTime(),
});
const millisecondsUntilNextMinute = (60 - currentUserLocalTime.seconds()) * 1000;
timerRef.current = setTimeout(() => {
setCurrentUserLocalTime(getCurrentUserLocalTime());
}, millisecondsUntilNextMinute);
}
}, [currentUserLocalTime, getCurrentUserLocalTime]);
puneetlath marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
updateCurrentTime();

return () => {
clearTimeout(timerRef.current);
};
}, [updateCurrentTime]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    };
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, what you have here might work the same - but I didn't think too much about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Timer is set when component initially loads
  2. 1 min later, currentTime is updated
  3. This causes useEffect to be called and set another 1 min timer
  4. 1 min later, currentTime is updated
  5. repeat, every min

Can y'all double-check me that the function makes sense now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can see what @burczu thinks as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it more and while I think the separate hook works, I think I prefer the other method for two reasons:

  1. It updates the clock at the top of the minute, rather than every 60 seconds from when the component is loaded
  2. It's the most simple/straightforward

If y'all strongly disagree with that, let me know. Otherwise, this is ready for re-review!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


render() {
return (
<View style={[styles.mb6, styles.detailsPageSectionContainer]}>
<Text style={[styles.textLabelSupporting, styles.mb1]} numberOfLines={1}>
{this.props.translate('detailsPage.localTime')}
</Text>
<Text numberOfLines={1}>
{this.state.currentUserLocalTime.format('LT')}
{' '}
{this.getTimezoneName()}
</Text>
</View>
);
}
return (
<View style={[styles.mb6, styles.detailsPageSectionContainer]}>
<Text style={[styles.textLabelSupporting, styles.mb1]} numberOfLines={1}>
{props.translate('detailsPage.localTime')}
</Text>
<Text numberOfLines={1}>
{currentUserLocalTime.format('LT')}
{' '}
{getTimezoneName()}
</Text>
</View>
);
}

AutoUpdateTime.propTypes = propTypes;
AutoUpdateTime.displayName = 'AutoUpdateTime';
export default withLocalize(AutoUpdateTime);