From 670991285a3e92e5fa69942bf7967bc16ef970ee Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 7 Dec 2018 15:26:05 -0800 Subject: [PATCH] Only use findDOMNode as a fallback (with a warning) --- src/ItemMeasurer.js | 57 +++++++++++++++++-- src/__tests__/DynamicSizeList.js | 45 ++++++++++++++- .../src/routes/examples/DynamicSizeList.js | 17 ++++-- 3 files changed, 107 insertions(+), 12 deletions(-) diff --git a/src/ItemMeasurer.js b/src/ItemMeasurer.js index ccab79b..86ad6c8 100644 --- a/src/ItemMeasurer.js +++ b/src/ItemMeasurer.js @@ -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); +if (process.env.NODE_ENV !== 'production') { + findDOMNodeWarningsSet = new Set(); +} + export default class ItemMeasurer extends Component { + _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 { // 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 { } 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 { } }; + _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); }; diff --git a/src/__tests__/DynamicSizeList.js b/src/__tests__/DynamicSizeList.js index 1c334e1..98bc0bb 100644 --- a/src/__tests__/DynamicSizeList.js +++ b/src/__tests__/DynamicSizeList.js @@ -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) => ( + + )); + beforeEach(() => { jest.useFakeTimers(); container = document.createElement('div'); itemSizes = [20, 25, 30, 35, 40]; - itemRenderer = jest.fn(({ style, ...rest }) => ( -
{JSON.stringify(rest, null, 2)}
+ itemRenderer = jest.fn(({ forwardedRef, style, ...rest }) => ( +
+ {JSON.stringify(rest, null, 2)} +
)); 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
{index}
; + } + } + + console.warn = jest.fn(); + render( + + {ItemRenderer} + , + 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( + + {ItemRenderer} + , + container + ); + expect(console.warn).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/website/src/routes/examples/DynamicSizeList.js b/website/src/routes/examples/DynamicSizeList.js index e2acbd9..ae5742f 100644 --- a/website/src/routes/examples/DynamicSizeList.js +++ b/website/src/routes/examples/DynamicSizeList.js @@ -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 (
( + +)); +const RefForwardedRow = React.forwardRef((props, 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} >
- {Row} + {RefForwardedRow}
@@ -197,7 +206,7 @@ export default class DynamicSizeList extends PureComponent { itemData={showText} width={300} > - {Column} + {RefForwardedColumn}