Skip to content

Commit

Permalink
[fix] Optimize ref merging
Browse files Browse the repository at this point in the history
Close #1746
Fix #1665
  • Loading branch information
necolas committed Sep 21, 2020
1 parent 4a70300 commit 20f889e
Show file tree
Hide file tree
Showing 17 changed files with 266 additions and 153 deletions.
3 changes: 2 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"env": {
"browser": true,
"es6": true,
"node": true
"node": true,
"jest": true
},
"globals": {

Expand Down
9 changes: 2 additions & 7 deletions packages/react-native-web/src/exports/Picker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import type { ViewProps } from '../View';

import createElement from '../createElement';
import setAndForwardRef from '../../modules/setAndForwardRef';
import useMergeRefs from '../../modules/useMergeRefs';
import usePlatformMethods from '../../hooks/usePlatformMethods';
import PickerItem from './PickerItem';
import StyleSheet, { type StyleObj } from '../StyleSheet';
Expand Down Expand Up @@ -47,12 +47,7 @@ const Picker = forwardRef<PickerProps, *>((props, forwardedRef) => {
} = props;

const hostRef = useRef(null);
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
hostRef.current = hostNode;
}
});
const setRef = useMergeRefs(forwardedRef, hostRef);

function handleChange(e: Object) {
const { selectedIndex, value } = e.target;
Expand Down
9 changes: 2 additions & 7 deletions packages/react-native-web/src/exports/Pressable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import type { ViewProps } from '../View';

import * as React from 'react';
import { forwardRef, memo, useMemo, useState, useRef } from 'react';
import setAndForwardRef from '../../modules/setAndForwardRef';
import useMergeRefs from '../../modules/useMergeRefs';
import usePressEvents from '../../hooks/usePressEvents';
import View from '../View';

Expand Down Expand Up @@ -97,12 +97,7 @@ function Pressable(props: Props, forwardedRef): React.Node {
const [pressed, setPressed] = useForceableState(testOnly_pressed === true);

const hostRef = useRef(null);
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
hostRef.current = hostNode;
}
});
const setRef = useMergeRefs(forwardedRef, hostRef);

const pressConfig = useMemo(
() => ({
Expand Down
9 changes: 2 additions & 7 deletions packages/react-native-web/src/exports/Text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { forwardRef, useContext, useRef } from 'react';
import createElement from '../createElement';
import css from '../StyleSheet/css';
import pick from '../../modules/pick';
import setAndForwardRef from '../../modules/setAndForwardRef';
import useElementLayout from '../../hooks/useElementLayout';
import useMergeRefs from '../../modules/useMergeRefs';
import usePlatformMethods from '../../hooks/usePlatformMethods';
import useResponderEvents from '../../hooks/useResponderEvents';
import StyleSheet from '../StyleSheet';
Expand Down Expand Up @@ -100,12 +100,7 @@ const Text = forwardRef<TextProps, *>((props, forwardedRef) => {

const hasTextAncestor = useContext(TextAncestorContext);
const hostRef = useRef(null);
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
hostRef.current = hostNode;
}
});
const setRef = useMergeRefs(forwardedRef, hostRef);

const classList = [
classes.text,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -633,4 +633,18 @@ describe('components/TextInput', () => {
const input = findInput(container);
expect(input.value).toEqual(value);
});

describe('imperative methods', () => {
test('node.clear()', () => {
const ref = React.createRef();
render(<TextInput ref={ref} />);
expect(typeof ref.current.clear).toBe('function');
});

test('node.isFocused()', () => {
const ref = React.createRef();
render(<TextInput ref={ref} />);
expect(typeof ref.current.isFocused).toBe('function');
});
});
});
62 changes: 31 additions & 31 deletions packages/react-native-web/src/exports/TextInput/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@

import type { TextInputProps } from './types';

import { forwardRef, useRef } from 'react';
import { forwardRef, useCallback, useMemo, useRef } from 'react';
import createElement from '../createElement';
import css from '../StyleSheet/css';
import pick from '../../modules/pick';
import setAndForwardRef from '../../modules/setAndForwardRef';
import useElementLayout from '../../hooks/useElementLayout';
import useLayoutEffect from '../../hooks/useLayoutEffect';
import useMergeRefs from '../../modules/useMergeRefs';
import usePlatformMethods from '../../hooks/usePlatformMethods';
import useResponderEvents from '../../hooks/useResponderEvents';
import StyleSheet from '../StyleSheet';
Expand Down Expand Up @@ -186,11 +186,31 @@ const TextInput = forwardRef<TextInputProps, *>((props, forwardedRef) => {
type = 'password';
}

const hostRef = useRef(null);
const dimensions = useRef({ height: null, width: null });
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
const hostRef = useRef(null);

const handleContentSizeChange = useCallback(() => {
const node = hostRef.current;
if (multiline && onContentSizeChange && node != null) {
const newHeight = node.scrollHeight;
const newWidth = node.scrollWidth;
if (newHeight !== dimensions.current.height || newWidth !== dimensions.current.width) {
dimensions.current.height = newHeight;
dimensions.current.width = newWidth;
onContentSizeChange({
nativeEvent: {
contentSize: {
height: dimensions.current.height,
width: dimensions.current.width
}
}
});
}
}
}, [hostRef, multiline, onContentSizeChange]);

const imperativeRef = useMemo(
() => hostNode => {
// TextInput needs to add more methods to the hostNode in addition to those
// added by `usePlatformMethods`. This is temporarily until an API like
// `TextInput.clear(hostRef)` is added to React Native.
Expand All @@ -203,13 +223,13 @@ const TextInput = forwardRef<TextInputProps, *>((props, forwardedRef) => {
hostNode.isFocused = function() {
return hostNode != null && TextInputState.currentlyFocusedField() === hostNode;
};
}
hostRef.current = hostNode;
if (hostRef.current != null) {
handleContentSizeChange();
}
}
});
},
[handleContentSizeChange]
);

const setRef = useMergeRefs(forwardedRef, hostRef, imperativeRef);

function handleBlur(e) {
TextInputState._currentlyFocusedNode = null;
Expand All @@ -219,26 +239,6 @@ const TextInput = forwardRef<TextInputProps, *>((props, forwardedRef) => {
}
}

function handleContentSizeChange() {
const node = hostRef.current;
if (multiline && onContentSizeChange && node != null) {
const newHeight = node.scrollHeight;
const newWidth = node.scrollWidth;
if (newHeight !== dimensions.current.height || newWidth !== dimensions.current.width) {
dimensions.current.height = newHeight;
dimensions.current.width = newWidth;
onContentSizeChange({
nativeEvent: {
contentSize: {
height: dimensions.current.height,
width: dimensions.current.width
}
}
});
}
}
}

function handleChange(e) {
const text = e.target.value;
e.nativeEvent.text = text;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import type { ViewProps } from '../View';

import * as React from 'react';
import { useCallback, useMemo, useState, useRef } from 'react';
import useMergeRefs from '../../modules/useMergeRefs';
import usePressEvents from '../../hooks/usePressEvents';
import setAndForwardRef from '../../modules/setAndForwardRef';
import StyleSheet from '../StyleSheet';
import View from '../View';

Expand Down Expand Up @@ -93,12 +93,7 @@ function TouchableHighlight(props: Props, forwardedRef): React.Node {
} = props;

const hostRef = useRef(null);
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
hostRef.current = hostNode;
}
});
const setRef = useMergeRefs(forwardedRef, hostRef);

const [extraStyles, setExtraStyles] = useState(
testOnly_pressed === true ? createExtraStyles(activeOpacity, underlayColor) : null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import type { ViewProps } from '../View';

import * as React from 'react';
import { useCallback, useMemo, useState, useRef } from 'react';
import useMergeRefs from '../../modules/useMergeRefs';
import usePressEvents from '../../hooks/usePressEvents';
import setAndForwardRef from '../../modules/setAndForwardRef';
import StyleSheet from '../StyleSheet';
import View from '../View';

Expand Down Expand Up @@ -51,12 +51,7 @@ function TouchableOpacity(props: Props, forwardedRef): React.Node {
} = props;

const hostRef = useRef(null);
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
hostRef.current = hostNode;
}
});
const setRef = useMergeRefs(forwardedRef, hostRef);

const [duration, setDuration] = useState('0s');
const [opacityOverride, setOpacityOverride] = useState(null);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import React from 'react';
import { render } from '@testing-library/react';
import TouchableWithoutFeedback from '../';
import View from '../../View';

describe('components/TouchableWithoutFeedback', () => {
test('forwards ref', () => {
const ref = jest.fn();
render(
<TouchableWithoutFeedback ref={ref}>
<View />
</TouchableWithoutFeedback>
);
expect(ref).toHaveBeenCalled();
});

test('forwards ref of child', () => {
const ref = jest.fn();
render(
<TouchableWithoutFeedback>
<View ref={ref} />
</TouchableWithoutFeedback>
);
expect(ref).toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type { ViewProps } from '../View';
import * as React from 'react';
import { useMemo, useRef } from 'react';
import pick from '../../modules/pick';
import setAndForwardRef from '../../modules/setAndForwardRef';
import useMergeRefs from '../../modules/useMergeRefs';
import usePressEvents from '../../hooks/usePressEvents';

export type Props = $ReadOnly<{|
Expand Down Expand Up @@ -115,20 +115,7 @@ function TouchableWithoutFeedback(props: Props, forwardedRef): React.Node {
supportedProps.accessible = accessible !== false;
supportedProps.accessibilityState = { disabled, ...props.accessibilityState };
supportedProps.focusable = focusable !== false && onPress !== undefined;
supportedProps.ref = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
const { ref } = element;
if (ref != null) {
if (typeof ref === 'function') {
ref(hostNode);
} else {
ref.current = hostNode;
}
}
hostRef.current = hostNode;
}
});
supportedProps.ref = useMergeRefs(forwardedRef, hostRef, element.ref);

const elementProps = Object.assign(supportedProps, pressEventHandlers);

Expand Down
9 changes: 2 additions & 7 deletions packages/react-native-web/src/exports/View/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { forwardRef, useContext, useRef } from 'react';
import createElement from '../createElement';
import css from '../StyleSheet/css';
import pick from '../../modules/pick';
import setAndForwardRef from '../../modules/setAndForwardRef';
import useElementLayout from '../../hooks/useElementLayout';
import useMergeRefs from '../../modules/useMergeRefs';
import usePlatformMethods from '../../hooks/usePlatformMethods';
import useResponderEvents from '../../hooks/useResponderEvents';
import StyleSheet from '../StyleSheet';
Expand Down Expand Up @@ -102,12 +102,7 @@ const View = forwardRef<ViewProps, *>((props, forwardedRef) => {

const hasTextAncestor = useContext(TextAncestorContext);
const hostRef = useRef(null);
const setRef = setAndForwardRef({
getForwardedRef: () => forwardedRef,
setLocalRef: hostNode => {
hostRef.current = hostNode;
}
});
const setRef = useMergeRefs(forwardedRef, hostRef);

const classList = [classes.view];
const style = StyleSheet.compose(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import * as React from 'react';
import mergeRefs from '..';
import { render } from '@testing-library/react';

describe('modules/mergeRefs', () => {
test('merges refs of different types', () => {
const ref = React.createRef(null);
let functionRefValue = null;
let hookRef;
function Component() {
const functionRef = x => {
functionRefValue = x;
};
hookRef = React.useRef(null);
return <div ref={mergeRefs(ref, hookRef, functionRef)} />;
}

render(<Component />);

expect(ref.current).not.toBe(null);
expect(hookRef.current).not.toBe(null);
expect(functionRefValue).not.toBe(null);
});
});
33 changes: 33 additions & 0 deletions packages/react-native-web/src/modules/mergeRefs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict
*/

import * as React from 'react';

export default function mergeRefs(...args: $ReadOnlyArray<React.ElementRef<any>>) {
return function forwardRef(node: HTMLElement | null) {
args.forEach((ref: React.ElementRef<any>) => {
if (ref == null) {
return;
}
if (typeof ref === 'function') {
ref(node);
return;
}
if (typeof ref === 'object') {
ref.current = node;
return;
}
console.error(
`mergeRefs cannot handle Refs of type boolean, number or string, received ref ${String(
ref
)}`
);
});
};
}
Loading

0 comments on commit 20f889e

Please sign in to comment.