Skip to content

Commit

Permalink
[fix] Revert ref memoization
Browse files Browse the repository at this point in the history
Memoizing refs caused unexpected regressions in some class component patterns.
The memoization is being reverted for now.

Fix #1749
  • Loading branch information
necolas committed Sep 24, 2020
1 parent af0d80a commit c60417a
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ describe('modules/mergeRefs', () => {

render(<Component />);

expect(ref.current).not.toBe(null);
expect(hookRef.current).not.toBe(null);
expect(functionRefValue).not.toBe(null);
expect(ref.current).toBeInstanceOf(HTMLDivElement);
expect(hookRef.current).toBeInstanceOf(HTMLDivElement);
expect(functionRefValue).toBeInstanceOf(HTMLDivElement);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,29 @@ describe('modules/useMergeRefs', () => {

afterEach(cleanup);

it('handles no refs', () => {
test('handles no refs', () => {
act(() => {
render(<TestComponent refs={[]} />);
});
});

test('merges any number of varying refs', () => {
const callbackRefs = Array(10).map(() => jest.fn());
const objectRefs = Array(10).map(() => ({ current: null }));
const nullRefs = Array(10).map(() => null);
const callbackRef1 = jest.fn();
const callbackRef2 = jest.fn();
const objectRef1 = React.createRef();
const objectRef2 = React.createRef();
const nullRef = null;

act(() => {
render(<TestComponent refs={[...callbackRefs, ...objectRefs, ...nullRefs]} />);
render(
<TestComponent refs={[callbackRef1, callbackRef2, objectRef1, objectRef2, nullRef]} />
);
});

callbackRefs.forEach(ref => {
expect(ref).toHaveBeenCalledTimes(1);
});

objectRefs.forEach(ref => {
expect(ref.current).toBeInstanceOf(HTMLButtonElement);
});
expect(callbackRef1).toHaveBeenCalledTimes(1);
expect(callbackRef2).toHaveBeenCalledTimes(1);
expect(objectRef1.current).toBeInstanceOf(HTMLDivElement);
expect(objectRef2.current).toBeInstanceOf(HTMLDivElement);
});

test('ref is called when ref changes', () => {
Expand All @@ -57,7 +58,7 @@ describe('modules/useMergeRefs', () => {
expect(nextRef).toHaveBeenCalled();
});

test('ref is not called for each rerender', () => {
test.skip('ref is not called for each rerender', () => {
const ref = jest.fn();
let rerender;

Expand All @@ -71,7 +72,7 @@ describe('modules/useMergeRefs', () => {
expect(ref).toHaveBeenCalledTimes(1);
});

test('ref is not called for props changes', () => {
test.skip('ref is not called for props changes', () => {
const ref = jest.fn();
let rerender;

Expand Down
9 changes: 2 additions & 7 deletions packages/react-native-web/src/modules/useMergeRefs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ import * as React from 'react';
import mergeRefs from '../mergeRefs';

export default function useMergeRefs(...args: $ReadOnlyArray<React.ElementRef<any>>) {
return React.useMemo(
() => mergeRefs(...args),
// Disable linter because args is always an array, and it is spread as
// arguments to mergeRefs correctly
// eslint-disable-next-line
[...args]
);
// TODO(memoize) #1755
return /*React.useMemo(() => */ mergeRefs(...args) /*, [args])*/;
}

0 comments on commit c60417a

Please sign in to comment.