Fix render loop in `<Modal>`, clean up ref merger code

This commit is contained in:
Evan Hahn 2021-10-01 18:53:00 -05:00 committed by GitHub
parent 1366d09f11
commit 893a77a3ad
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 36 additions and 39 deletions

View File

@ -15,7 +15,7 @@ import classNames from 'classnames';
import * as grapheme from '../util/grapheme';
import { LocalizerType } from '../types/Util';
import { getClassNamesFor } from '../util/getClassNamesFor';
import { multiRef } from '../util/multiRef';
import { refMerger } from '../util/refMerger';
export type PropsType = {
countLength?: (value: string) => number;
@ -173,7 +173,7 @@ export const Input = forwardRef<
onKeyDown: handleKeyDown,
onPaste: handlePaste,
placeholder,
ref: multiRef<HTMLInputElement | HTMLTextAreaElement | null>(
ref: refMerger<HTMLInputElement | HTMLTextAreaElement | null>(
ref,
innerRef
),

View File

@ -12,6 +12,7 @@ import { Theme } from '../util/theme';
import { getClassNamesFor } from '../util/getClassNamesFor';
import { useAnimated } from '../hooks/useAnimated';
import { useHasWrapped } from '../hooks/useHasWrapped';
import { useRefMerger } from '../hooks/useRefMerger';
type PropsType = {
children: ReactNode;
@ -85,6 +86,8 @@ export function ModalWindow({
}: Readonly<PropsType>): JSX.Element {
const modalRef = useRef<HTMLDivElement | null>(null);
const refMerger = useRefMerger();
const bodyRef = useRef<HTMLDivElement | null>(null);
const [scrolled, setScrolled] = useState(false);
const [hasOverflow, setHasOverflow] = useState(false);
@ -155,10 +158,7 @@ export function ModalWindow({
const scrollTop = bodyRef.current?.scrollTop || 0;
setScrolled(scrollTop > 2);
}}
ref={bodyEl => {
measureRef(bodyEl);
bodyRef.current = bodyEl;
}}
ref={refMerger(measureRef, bodyRef)}
>
{children}
</div>

View File

@ -6,7 +6,7 @@ import classNames from 'classnames';
import { noop } from 'lodash';
import { Manager, Reference, Popper } from 'react-popper';
import { Theme, themeClassName } from '../util/theme';
import { multiRef } from '../util/multiRef';
import { refMerger } from '../util/refMerger';
import { offsetDistanceModifier } from '../util/popperUtil';
type EventWrapperPropsType = {
@ -52,7 +52,7 @@ const TooltipEventWrapper = React.forwardRef<
<span
onFocus={on}
onBlur={off}
ref={multiRef<HTMLSpanElement>(ref, wrapperRef)}
ref={refMerger<HTMLSpanElement>(ref, wrapperRef)}
>
{children}
</span>

View File

@ -1,30 +1,6 @@
// Copyright 2019-2020 Signal Messenger, LLC
// Copyright 2019-2021 Signal Messenger, LLC
// SPDX-License-Identifier: AGPL-3.0-only
import { MutableRefObject, Ref } from 'react';
import { isFunction } from 'lodash';
import memoizee from 'memoizee';
export function cleanId(id: string): string {
return id.replace(/[^\u0020-\u007e\u00a0-\u00ff]/g, '_');
}
// Memoizee makes this difficult.
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export const createRefMerger = () =>
memoizee(
<T>(...refs: Array<Ref<T>>) => {
return (t: T) => {
refs.forEach(r => {
if (isFunction(r)) {
r(t);
} else if (r) {
// Using a MutableRefObject as intended
// eslint-disable-next-line no-param-reassign
(r as MutableRefObject<T>).current = t;
}
});
};
},
{ length: false, max: 1 }
);

View File

@ -62,7 +62,7 @@ import {
ConversationColorType,
CustomColorType,
} from '../../types/Colors';
import { createRefMerger } from '../_util';
import { createRefMerger } from '../../util/refMerger';
import { emojiToData } from '../emoji/lib';
import type { SmartReactionPicker } from '../../state/smart/ReactionPicker';
import { getCustomColorStyle } from '../../util/getCustomColorStyle';

View File

@ -13,7 +13,7 @@ import Measure, { MeasuredComponentProps } from 'react-measure';
import { LocalizerType } from '../../../../types/Util';
import { assert } from '../../../../util/assert';
import { getOwn } from '../../../../util/getOwn';
import { multiRef } from '../../../../util/multiRef';
import { refMerger } from '../../../../util/refMerger';
import { useRestoreFocus } from '../../../../hooks/useRestoreFocus';
import { missingCaseError } from '../../../../util/missingCaseError';
import { filterAndSortConversationsByTitle } from '../../../../util/filterAndSortConversations';
@ -146,7 +146,7 @@ export const ChooseGroupMembersModal: FunctionComponent<PropsType> = ({
confirmAdds();
}
}}
ref={multiRef<HTMLInputElement>(inputRef, focusRef)}
ref={refMerger<HTMLInputElement>(inputRef, focusRef)}
value={searchTerm}
/>
{Boolean(selectedContacts.length) && (

8
ts/hooks/useRefMerger.ts Normal file
View File

@ -0,0 +1,8 @@
// Copyright 2021 Signal Messenger, LLC
// SPDX-License-Identifier: AGPL-3.0-only
import { useMemo } from 'react';
import { createRefMerger } from '../util/refMerger';
export const useRefMerger = (): ReturnType<typeof createRefMerger> =>
useMemo(createRefMerger, []);

View File

@ -1,9 +1,18 @@
// Copyright 2021 Signal Messenger, LLC
// SPDX-License-Identifier: AGPL-3.0-only
import { Ref } from 'react';
import type { MutableRefObject, Ref } from 'react';
import memoizee from 'memoizee';
export function multiRef<T>(...refs: Array<Ref<T>>): (topLevelRef: T) => void {
/**
* Merges multiple refs.
*
* Returns a new function each time, which may cause unnecessary re-renders. Try
* `createRefMerger` if you want to cache the function.
*/
export function refMerger<T>(
...refs: Array<Ref<unknown>>
): (topLevelRef: T) => void {
return (el: T) => {
refs.forEach(ref => {
// This is a simplified version of [what React does][0] to set a ref.
@ -15,8 +24,12 @@ export function multiRef<T>(...refs: Array<Ref<T>>): (topLevelRef: T) => void {
// not be `readonly`. That's why we do this cast. See [the React source][1].
// [1]: https://github.com/facebook/react/blob/29b7b775f2ecf878eaf605be959d959030598b07/packages/shared/ReactTypes.js#L78-L80
// eslint-disable-next-line no-param-reassign
(ref as React.MutableRefObject<T>).current = el;
(ref as MutableRefObject<T>).current = el;
}
});
};
}
export function createRefMerger(): typeof refMerger {
return memoizee(refMerger, { length: false, max: 1 });
}