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

remove buggy useEffect usage #1316

Merged
merged 6 commits into from
Dec 17, 2024
Merged

remove buggy useEffect usage #1316

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
38a07dc
remove buggy useEffect usage
daniel-heppner-ibigroup Dec 4, 2024
73b17e1
remove two more useEffect hooks
daniel-heppner-ibigroup Dec 4, 2024
6ef1587
adjust initial state loading
daniel-heppner-ibigroup Dec 4, 2024
219f41f
Merge branch 'dev' into cleanup-calltaker-useeffect
daniel-heppner-ibigroup Dec 5, 2024
55f1f93
Merge branch 'dev' into cleanup-calltaker-useeffect
miles-grant-ibigroup Dec 13, 2024
caf809c
useCallback in input props, add comments and eslint disable to effect…
daniel-heppner-ibigroup Dec 17, 2024
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
197 changes: 111 additions & 86 deletions lib/components/form/call-taker/date-time-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@ import { IntlShape, useIntl } from 'react-intl'
import { isMatch, parse } from 'date-fns'
import { OverlayTrigger, Tooltip } from 'react-bootstrap'
import coreUtils from '@opentripplanner/core-utils'
import React, { useEffect, useRef, useState } from 'react'
import React, {
ChangeEvent,
useCallback,
useEffect,
useRef,
useState
} from 'react'

import { AppReduxState, FilterType, SortType } from '../../../util/state-types'
import { DepartArriveTypeMap, DepartArriveValue } from '../date-time-modal'
Expand Down Expand Up @@ -69,6 +75,30 @@ const safeFormat = (date: Date | '', time: string, options?: OptionsWithTZ) => {
}
return ''
}
/**
* Parse a time input expressed in the agency time zone.
* @returns A date if the parsing succeeded, or null.
*/
const parseInputAsTime = (
homeTimezone: string,
timeInput: string = getCurrentTime(homeTimezone),
date: string = getCurrentDate(homeTimezone)
) => {
if (!timeInput) timeInput = getCurrentTime(homeTimezone)

// Match one of the supported time formats
const matchedTimeFormat = SUPPORTED_TIME_FORMATS.find((timeFormat) =>
isMatch(timeInput, timeFormat)
)
if (matchedTimeFormat) {
const resolvedDateTime = format(
parse(timeInput, matchedTimeFormat, new Date()),
'HH:mm:ss'
)
return toDate(`${date}T${resolvedDateTime}`)
}
return ''
}

type Props = {
date?: string
Expand Down Expand Up @@ -121,69 +151,36 @@ const DateTimeOptions = ({
)
const [date, setDate] = useState<string | undefined>(initialDate)
const [time, setTime] = useState<string | undefined>(initialTime)
const [typedTime, setTypedTime] = useState<string | undefined>(initialTime)
const [typedTime, setTypedTime] = useState<string | undefined>(
safeFormat(parseInputAsTime(homeTimezone, time, date), timeFormat, {
timeZone: homeTimezone
})
)

const timeRef = useRef(null)

const intl = useIntl()

/**
* Parse a time input expressed in the agency time zone.
* @returns A date if the parsing succeeded, or null.
*/
const parseInputAsTime = (
daniel-heppner-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
timeInput: string = getCurrentTime(homeTimezone),
date: string = getCurrentDate(homeTimezone)
) => {
if (!timeInput) timeInput = getCurrentTime(homeTimezone)

// Match one of the supported time formats
const matchedTimeFormat = SUPPORTED_TIME_FORMATS.find((timeFormat) =>
isMatch(timeInput, timeFormat)
)
if (matchedTimeFormat) {
const resolvedDateTime = format(
parse(timeInput, matchedTimeFormat, new Date()),
'HH:mm:ss'
)
return toDate(`${date}T${resolvedDateTime}`)
}
return ''
}

const dateTime = parseInputAsTime(time, date)
const dateTime = parseInputAsTime(homeTimezone, time, date)

// Update state when external state is updated
useEffect(() => {
if (initialDate !== date) setDate(initialDate)
if (initialTime !== time) {
setTime(initialTime)
handleTimeChange(initialTime || '')
}
// This effect is design to flow from state to component only
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [initialTime, initialDate])
Copy link
Collaborator

Choose a reason for hiding this comment

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

eslint is highlighting missing dependencies here: date, time, and handleTimeChange, could you add them to the dependency array as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is these useEffect hooks are propagating updates from the store into the component, adding those deps would cause a loop. Otherwise I would have just replaced them with callbacks

Copy link
Collaborator

Choose a reason for hiding this comment

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

@daniel-heppner-ibigroup Okay, could you add a comment about the undesired side effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one in caf809c


useEffect(() => {
// Don't update if still typing
if (timeRef.current !== document.activeElement) {
setTypedTime(
safeFormat(dateTime, timeFormat, {
timeZone: homeTimezone
}) ||
// TODO: there doesn't seem to be an intl object present?
'Invalid Time'
)
}
}, [time])

useEffect(() => {
if (initialDepartArrive && departArrive !== initialDepartArrive) {
setDepartArrive(initialDepartArrive)
}
// This effect is design to flow from state to component only
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [initialDepartArrive])
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we are in fixing dependency arrays, could you adding departArrive in here too per eslint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, causes a loop


useEffect(() => {
if (departArrive === 'NOW') setTypedTime('')
}, [departArrive])

// Handler for setting the query parameters
useEffect(() => {
if (safeFormat(dateTime, OTP_API_DATE_FORMAT, {}) !== '' && setQueryParam) {
Expand All @@ -197,42 +194,64 @@ const DateTimeOptions = ({
})
})
}

if (
syncSortWithDepartArrive &&
DepartArriveTypeMap[departArrive] !== sort.type
) {
importedUpdateItineraryFilter({
sort: {
...sort,
type: DepartArriveTypeMap[departArrive]
}
})
}
}, [dateTime, departArrive, homeTimezone, setQueryParam])

// Handler for updating the time and date fields when NOW is selected
useEffect(() => {
if (departArrive === 'NOW') {
setTime(getCurrentTime(homeTimezone))
setDate(getCurrentDate(homeTimezone))
setTypedTime(
safeFormat(dateTime, timeFormat, {
timeZone: homeTimezone
const handleDepartArriveChange = useCallback(
(e: ChangeEvent<HTMLSelectElement>) => {
const newValue = e.target.value as DepartArriveValue
setDepartArrive(newValue)

// Handler for updating the time and date fields when NOW is selected
if (newValue === 'NOW') {
handleTimeChange(getCurrentTime(homeTimezone))
setDate(getCurrentDate(homeTimezone))
setTypedTime(
safeFormat(dateTime, timeFormat, {
timeZone: homeTimezone
})
)
}

// Update sort type if needed
if (
syncSortWithDepartArrive &&
DepartArriveTypeMap[newValue] !== sort.type
) {
importedUpdateItineraryFilter({
sort: {
...sort,
type: DepartArriveTypeMap[newValue]
}
})
)
}
}, [departArrive, setTime, setDate, homeTimezone])
}
},
[syncSortWithDepartArrive, sort, importedUpdateItineraryFilter]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, eslint is suggesting to add dateTime, handleTimeChange, homeTimezone to the dependency array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good note, added them.

)

const unsetNow = () => {
const unsetNow = useCallback(() => {
if (departArrive === 'NOW') setDepartArrive('DEPART')
}
}, [departArrive])

const handleTimeChange = useCallback(
(newTime: string) => {
setTime(newTime)
// Only update typed time if not actively typing
if (timeRef.current !== document.activeElement) {
setTypedTime(
safeFormat(dateTime, timeFormat, {
timeZone: homeTimezone
}) || 'Invalid Time'
)
}
},
[dateTime, timeFormat, homeTimezone]
)

return (
<>
<select
onBlur={(e) => setDepartArrive(e.target.value as DepartArriveValue)}
onChange={(e) => setDepartArrive(e.target.value as DepartArriveValue)}
onBlur={handleDepartArriveChange}
onChange={handleDepartArriveChange}
onKeyDown={onKeyDown}
value={departArrive}
>
Expand All @@ -257,11 +276,14 @@ const DateTimeOptions = ({
>
<input
className="datetime-slim"
onChange={(e) => {
setTime(e.target.value)
setTypedTime(e.target.value)
unsetNow()
}}
onChange={useCallback(
(e) => {
handleTimeChange(e.target.value)
setTypedTime(e.target.value)
unsetNow()
},
[handleTimeChange, setTypedTime, unsetNow]
)}
onFocus={(e) => e.target.select()}
onKeyDown={onKeyDown}
ref={timeRef}
Expand All @@ -278,15 +300,18 @@ const DateTimeOptions = ({
<input
className="datetime-slim"
disabled={!dateTime}
onChange={(e) => {
if (!e.target.value) {
e.preventDefault()
// TODO: prevent selection from advancing to next field
return
}
setDate(e.target.value)
unsetNow()
}}
onChange={useCallback(
(e) => {
if (!e.target.value) {
e.preventDefault()
// TODO: prevent selection from advancing to next field
return
}
setDate(e.target.value)
unsetNow()
},
[unsetNow, setDate]
)}
onKeyDown={onKeyDown}
style={{
fontSize: '14px',
Expand Down
Loading