Only use findDOMNode as a fallback (with a warning)

This commit is contained in:
Brian Vaughn
2018-12-07 15:26:05 -08:00
parent fe88aebbee
commit 670991285a
3 changed files with 107 additions and 12 deletions
+52 -5
View File
@@ -1,6 +1,6 @@
// @flow
import { Component } from 'react';
import { cloneElement, Component } from 'react';
import { findDOMNode } from 'react-dom';
import type { Direction } from './createListComponent';
@@ -44,13 +44,39 @@ type ItemMeasurerProps = {|
size: number,
|};
let findDOMNodeWarningsSet = ((null: any): Set<any>);
if (process.env.NODE_ENV !== 'production') {
findDOMNodeWarningsSet = new Set();
}
export default class ItemMeasurer extends Component<ItemMeasurerProps, void> {
_didProvideValidRef: boolean = false;
_node: HTMLElement | null = null;
_resizeObserver: ResizeObserver | null = null;
componentDidMount() {
const node = ((findDOMNode(this): any): HTMLElement);
this._node = node;
if (process.env.NODE_ENV !== 'production') {
if (!this._didProvideValidRef) {
const { item } = this.props;
const displayName =
item && item.type
? item.type.displayName || item.type.name || '(unknown)'
: '(unknown)';
if (!findDOMNodeWarningsSet.has(displayName)) {
findDOMNodeWarningsSet.add(displayName);
console.warn(
'DynamicSizeList item renderers should attach a ref to the topmost HTMLElement they render. ' +
`The item renderer "${displayName}" did not attach a ref to a valid HTMLElement. ` +
'findDOMNode() will be used as a fallback, but is slower and more error prone than using a ref.\n\n' +
'Learn more about ref forwarding: ' +
'https://reactjs.org/docs/forwarding-refs.html#forwarding-refs-to-dom-components'
);
}
}
}
// Force sync measure for the initial mount.
// This is necessary to support the DynamicSizeList layout logic.
@@ -60,7 +86,9 @@ export default class ItemMeasurer extends Component<ItemMeasurerProps, void> {
// Watch for resizes due to changed content,
// Or changes in the size of the parent container.
this._resizeObserver = new ResizeObserver(this._onResize);
this._resizeObserver.observe(node);
if (this._node !== null) {
this._resizeObserver.observe(this._node);
}
}
}
@@ -71,7 +99,9 @@ export default class ItemMeasurer extends Component<ItemMeasurerProps, void> {
}
render() {
return this.props.item;
return cloneElement(this.props.item, {
ref: this._refSetter,
});
}
_measureItem = (isCommitPhase: boolean) => {
@@ -101,6 +131,23 @@ export default class ItemMeasurer extends Component<ItemMeasurerProps, void> {
}
};
_refSetter = (ref: any) => {
if (this._resizeObserver !== null && this._node !== null) {
this._resizeObserver.unobserve(this._node);
}
if (ref instanceof HTMLElement) {
this._didProvideValidRef = true;
this._node = ref;
} else if (ref !== null) {
this._node = ((findDOMNode(ref): any): HTMLElement);
}
if (this._resizeObserver !== null && this._node !== null) {
this._resizeObserver.observe(this._node);
}
};
_onResize = () => {
this._measureItem(false);
};
+42 -3
View File
@@ -1,5 +1,6 @@
import React, {
createRef,
forwardRef,
PureComponent,
unstable_Profiler as Profiler,
} from 'react';
@@ -39,19 +40,25 @@ describe('DynamicSizeList', () => {
}
}
const RefForwarder = forwardRef((props, ref) => (
<PureItemRenderer {...props} forwardedRef={ref} />
));
beforeEach(() => {
jest.useFakeTimers();
container = document.createElement('div');
itemSizes = [20, 25, 30, 35, 40];
itemRenderer = jest.fn(({ style, ...rest }) => (
<div style={style}>{JSON.stringify(rest, null, 2)}</div>
itemRenderer = jest.fn(({ forwardedRef, style, ...rest }) => (
<div ref={forwardedRef} style={style}>
{JSON.stringify(rest, null, 2)}
</div>
));
onItemsRendered = jest.fn();
innerRef = createRef();
defaultProps = {
children: PureItemRenderer,
children: RefForwarder,
estimatedItemSize: 25,
height: 100,
innerRef,
@@ -100,4 +107,36 @@ describe('DynamicSizeList', () => {
expect(onRender.mock.calls).toHaveLength(2);
});
describe('ref forwarding', () => {
it('should warn if ref is not forwarded', () => {
class ItemRenderer extends PureComponent {
render() {
const { index, style } = this.props;
return <div style={style}>{index}</div>;
}
}
console.warn = jest.fn();
render(
<DynamicSizeList {...defaultProps} itemCount={5}>
{ItemRenderer}
</DynamicSizeList>,
container
);
expect(console.warn).toHaveBeenCalledTimes(1);
expect(console.warn.mock.calls[0][0]).toContain(
'The item renderer "ItemRenderer" did not attach a ref'
);
// It should only warn once per item renderer type to avoid spamming the console.
render(
<DynamicSizeList {...defaultProps} itemCount={10}>
{ItemRenderer}
</DynamicSizeList>,
container
);
expect(console.warn).toHaveBeenCalledTimes(1);
});
});
});
+13 -4
View File
@@ -49,12 +49,13 @@ class Row extends PureComponent {
};
render() {
const { index, style } = this.props;
const { index, forwardedRef, style } = this.props;
const item = items[index];
return (
<div
className={index % 2 ? styles.DynamicRowOdd : styles.DynamicRowEven}
ref={forwardedRef}
style={style}
>
<div
@@ -74,6 +75,13 @@ class Row extends PureComponent {
}
}
const RefForwardedColumn = React.forwardRef((props, ref) => (
<Column {...props} forwardedRef={ref} />
));
const RefForwardedRow = React.forwardRef((props, ref) => (
<Row {...props} forwardedRef={ref} />
));
class Column extends PureComponent {
toggleExpanded = () => {
const { index } = this.props;
@@ -85,7 +93,7 @@ class Column extends PureComponent {
};
render() {
const { data: showText, index, style } = this.props;
const { data: showText, forwardedRef, index, style } = this.props;
const item = items[index];
return (
@@ -93,6 +101,7 @@ class Column extends PureComponent {
className={
index % 2 ? styles.DynamicColumnOdd : styles.DynamicColumnEven
}
ref={forwardedRef}
style={style}
>
<div
@@ -171,7 +180,7 @@ export default class DynamicSizeList extends PureComponent {
itemCount={items.length}
width={halfSize ? 200 : 300}
>
{Row}
{RefForwardedRow}
</List>
</ProfiledExample>
<div className={styles.ExampleCode}>
@@ -197,7 +206,7 @@ export default class DynamicSizeList extends PureComponent {
itemData={showText}
width={300}
>
{Column}
{RefForwardedColumn}
</List>
</ProfiledExample>
<div className={styles.ExampleCode}>