-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix React StrictMode Calendar #4035
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.
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
Changes from all commits
4cf6471
6e88149
c430e11
a3f5942
9858a27
ffaa489
3e3cf84
37a159d
f80990a
cd341f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
|
|
||
| import {DateFormatter} from '@internationalized/date'; | ||
| import {useLocale} from './context'; | ||
| import {useMemo, useRef} from 'react'; | ||
| import {useMemo, useState} from 'react'; | ||
|
|
||
| export interface DateFormatterOptions extends Intl.DateTimeFormatOptions { | ||
| calendar?: string | ||
|
|
@@ -24,16 +24,18 @@ export interface DateFormatterOptions extends Intl.DateTimeFormatOptions { | |
| * @param options - Formatting options. | ||
| */ | ||
| export function useDateFormatter(options?: DateFormatterOptions): DateFormatter { | ||
| let {locale} = useLocale(); | ||
|
|
||
| // Reuse last options object if it is shallowly equal, which allows the useMemo result to also be reused. | ||
| let lastOptions = useRef(null); | ||
| if (options && lastOptions.current && isEqual(options, lastOptions.current)) { | ||
| options = lastOptions.current; | ||
| let [stabilizedOptions, setStabilizedOptions] = useState(options); | ||
| if (!stabilizedOptions || (options && !isEqual(stabilizedOptions, options))) { | ||
| // early render bail because setState in render | ||
| setStabilizedOptions(options); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A ref is actually safe here because the values are equivalent, this is just an optimization. So even if a render were to be thrown away, the behavior should be the same. Updated in #4564 |
||
| } | ||
|
|
||
| lastOptions.current = options; | ||
| let formattedDate = useMemo(() => new DateFormatter(locale, stabilizedOptions), [locale, stabilizedOptions]); | ||
|
|
||
| let {locale} = useLocale(); | ||
| return useMemo(() => new DateFormatter(locale, options), [locale, options]); | ||
| return formattedDate; | ||
| } | ||
|
|
||
| function isEqual(a: DateFormatterOptions, b: DateFormatterOptions) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,20 +64,25 @@ export class SyntheticFocusEvent<Target = Element> implements ReactFocusEvent<Ta | |
| export function useSyntheticBlurEvent<Target = Element>(onBlur: (e: ReactFocusEvent<Target>) => void) { | ||
| let stateRef = useRef({ | ||
| isFocused: false, | ||
| onBlur, | ||
| observer: null as MutationObserver | ||
| observer: null as MutationObserver, | ||
| target: null as Element, | ||
| listener: null as (e: FocusEvent) => void | ||
| }); | ||
| stateRef.current.onBlur = onBlur; | ||
|
|
||
| // Clean up MutationObserver on unmount. See below. | ||
| // eslint-disable-next-line arrow-body-style | ||
| useLayoutEffect(() => { | ||
| const state = stateRef.current; | ||
| return () => { | ||
| if (state.observer) { | ||
| state.observer.disconnect(); | ||
| state.observer = null; | ||
| } | ||
| if (state.target && state.listener) { | ||
| state.target.removeEventListener('focusout', state.listener); | ||
| } | ||
| state.target = null; | ||
| state.listener = null; | ||
| state.isFocused = false; | ||
| }; | ||
| }, []); | ||
|
|
||
|
|
@@ -101,7 +106,7 @@ export function useSyntheticBlurEvent<Target = Element>(onBlur: (e: ReactFocusEv | |
|
|
||
| if (target.disabled) { | ||
| // For backward compatibility, dispatch a (fake) React synthetic event. | ||
| stateRef.current.onBlur?.(new SyntheticFocusEvent('blur', e)); | ||
| onBlur?.(new SyntheticFocusEvent('blur', e)); | ||
| } | ||
|
|
||
| // We no longer need the MutationObserver once the target is blurred. | ||
|
|
@@ -111,10 +116,19 @@ export function useSyntheticBlurEvent<Target = Element>(onBlur: (e: ReactFocusEv | |
| } | ||
| }; | ||
|
|
||
| if (stateRef.current.target && stateRef.current.listener) { | ||
| stateRef.current.target.removeEventListener('focusout', stateRef.current.listener); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do some parts of this function use |
||
| } | ||
| target.addEventListener('focusout', onBlurHandler, {once: true}); | ||
| stateRef.current.target = target; | ||
| stateRef.current.listener = onBlurHandler; | ||
|
|
||
| if (stateRef.current.observer) { | ||
| stateRef.current.observer.disconnect(); | ||
| stateRef.current.observer = null; | ||
| } | ||
| stateRef.current.observer = new MutationObserver(() => { | ||
| if (stateRef.current.isFocused && target.disabled) { | ||
| if (stateRef.current.isFocused && target.disabled && document.contains(target)) { | ||
| stateRef.current.observer.disconnect(); | ||
| target.dispatchEvent(new FocusEvent('blur')); | ||
| target.dispatchEvent(new FocusEvent('focusout', {bubbles: true})); | ||
|
|
@@ -123,5 +137,5 @@ export function useSyntheticBlurEvent<Target = Element>(onBlur: (e: ReactFocusEv | |
|
|
||
| stateRef.current.observer.observe(target, {attributes: true, attributeFilter: ['disabled']}); | ||
| } | ||
| }, []); | ||
| }, [onBlur]); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There were a lot of changes in this file that I wasn't sure where they came from. In #4564, the only change I needed was to make |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ import { | |
| import {CalendarProps, DateValue} from '@react-types/calendar'; | ||
| import {CalendarState} from './types'; | ||
| import {useControlledState} from '@react-stately/utils'; | ||
| import {useMemo, useRef, useState} from 'react'; | ||
| import {useEffect, useMemo, useRef, useState} from 'react'; | ||
|
|
||
| export interface CalendarStateOptions<T extends DateValue = DateValue> extends CalendarProps<T> { | ||
| /** The locale to display and edit the value according to. */ | ||
|
|
@@ -113,12 +113,15 @@ export function useCalendarState<T extends DateValue = DateValue>(props: Calenda | |
|
|
||
| // Reset focused date and visible range when calendar changes. | ||
| let lastCalendarIdentifier = useRef(calendar.identifier); | ||
| if (calendar.identifier !== lastCalendarIdentifier.current) { | ||
| let newFocusedDate = toCalendar(focusedDate, calendar); | ||
| setStartDate(alignCenter(newFocusedDate, visibleDuration, locale, minValue, maxValue)); | ||
| setFocusedDate(newFocusedDate); | ||
| lastCalendarIdentifier.current = calendar.identifier; | ||
| } | ||
| let calIdentifier = calendar.identifier; | ||
| useEffect(() => { | ||
| if (calendar.identifier !== lastCalendarIdentifier.current) { | ||
| let newFocusedDate = toCalendar(focusedDate, calendar); | ||
| setStartDate(alignCenter(newFocusedDate, visibleDuration, locale, minValue, maxValue)); | ||
| setFocusedDate(newFocusedDate); | ||
| lastCalendarIdentifier.current = calIdentifier; | ||
| } | ||
| }, [calendar, focusedDate, visibleDuration, locale, minValue, maxValue, setFocusedDate, calIdentifier]); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do these in render still, we just need to make |
||
|
|
||
| if (isInvalid(focusedDate, minValue, maxValue)) { | ||
| // If the focused date was moved to an invalid value, it can't be focused, so constrain it. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ import {RangeCalendarProps} from '@react-types/calendar'; | |
| import {RangeValue} from '@react-types/shared'; | ||
| import {useCalendarState} from './useCalendarState'; | ||
| import {useControlledState} from '@react-stately/utils'; | ||
| import {useMemo, useRef, useState} from 'react'; | ||
| import {useEffect, useMemo, useRef, useState} from 'react'; | ||
|
|
||
| export interface RangeCalendarStateOptions<T extends DateValue = DateValue> extends RangeCalendarProps<T> { | ||
| /** The locale to display and edit the value according to. */ | ||
|
|
@@ -92,10 +92,12 @@ export function useRangeCalendarState<T extends DateValue = DateValue>(props: Ra | |
|
|
||
| // If the visible range changes, we need to update the available range. | ||
| let lastVisibleRange = useRef(calendar.visibleRange); | ||
| if (!isEqualDay(calendar.visibleRange.start, lastVisibleRange.current.start) || !isEqualDay(calendar.visibleRange.end, lastVisibleRange.current.end)) { | ||
| updateAvailableRange(anchorDate); | ||
| lastVisibleRange.current = calendar.visibleRange; | ||
| } | ||
| useEffect(() => { | ||
| if (!isEqualDay(calendar.visibleRange.start, lastVisibleRange.current.start) || !isEqualDay(calendar.visibleRange.end, lastVisibleRange.current.end)) { | ||
| updateAvailableRange(anchorDate); | ||
| lastVisibleRange.current = calendar.visibleRange; | ||
| } | ||
| }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to avoid an extra render here too. This one seems like |
||
|
|
||
| let setAnchorDate = (date: CalendarDate) => { | ||
| if (date) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
|
|
||
| import {Calendar, now, Time, toCalendar, toCalendarDate, toCalendarDateTime} from '@internationalized/date'; | ||
| import {DatePickerProps, DateValue, Granularity, TimeValue} from '@react-types/datepicker'; | ||
| import {useRef} from 'react'; | ||
| import {useEffect, useRef} from 'react'; | ||
|
|
||
| export function isInvalid(value: DateValue, minValue: DateValue, maxValue: DateValue) { | ||
| return value != null && ( | ||
|
|
@@ -131,11 +131,15 @@ export function createPlaceholderDate(placeholderValue: DateValue, granularity: | |
| export function useDefaultProps(v: DateValue, granularity: Granularity): [Granularity, string] { | ||
| // Compute default granularity and time zone from the value. If the value becomes null, keep the last values. | ||
| let lastValue = useRef(v); | ||
| if (v) { | ||
| lastValue.current = v; | ||
| useEffect(() => { | ||
| if (v) { | ||
| lastValue.current = v; | ||
| } | ||
| }, [v]); | ||
|
|
||
| if (!v) { | ||
| v = lastValue.current; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is still a read in render...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated this in #4564 to use state instead of a ref. The state should only update if the time zone or granularity change, which should be rare. |
||
| } | ||
|
|
||
| v = lastValue.current; | ||
| let defaultTimeZone = (v && 'timeZone' in v ? v.timeZone : undefined); | ||
| granularity = granularity || (v && 'minute' in v ? 'minute' : 'day'); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this too late? If a browser were to fire
onBlurwhen a button becomes disabled, that would happen before this layout effect, and thusnextFocused.currentwould already be false and we would never callsetFocused.I think the point of this was to avoid focus being lost when the button becomes disabled. The safest thing might be to change
nextFocusedintouseStateinstead ofuseRef. But really, is what we currently have any less safe than something like this?Not suggesting we do that, just wondering if what we have is really bad or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you can see this in chrome. Go here, go forward a month, and then tab to the previous button and press Enter. Focus gets lost rather than going back to the calendar, because the onBlur event runs first. Doesn't happen in JSDom or in other browsers.
I fixed this in #4564 by using state instead of a ref.