From d2f314af75b63443db23e131aaf93c2d064e4f44 Mon Sep 17 00:00:00 2001 From: Kacie Bawiec Date: Thu, 26 Mar 2020 16:47:39 -0700 Subject: [PATCH] Make ScrollView use ForwardRef Summary: Have ScrollView use forwardRef so that the host component methods like `measure` and `measureLayout` are available without having to call `getNativeScrollRef`. Instead, you can use `` and directly call all methods of ScrollView and host components on `myRef`. Previous usage: ``` const myRef = React.createRef>(); const innerViewRef = myRef.current.getNativeScrollRef(); innerViewRef.measure(); ``` New usage: ``` const myRef = React.createRef>(); // now, myRef.current can be used directly as the ref myRef.current.measure(); myRef.current.measureLayout(); // Additionally, myRef still has access to ScrollView methods myRef.current.scrollTo(...); ``` Changes: * Added deprecation warnings to ScrollView methods `getNativeScrollRef`, `getScrollableNode`, and `getScrollResponder` * Added the forwardRef call to create `ForwardedScrollView` - this takes in `ref` and passes it into the class ScrollView as `scrollViewRef`. * Forwarded the ref to the native scroll view using `setAndForwardRef`. * Added statics onto `ForwardedScrollView` so that `ScrollView.Context` can still be accessed. * Added type `ScrollViewImperativeMethods`, which lists the public methods of ScrollView. * Converted all public methods of ScrollView to arrow functions. This is because they need to be bound to the forwarded ref. * Bound all public methods of ScrollView to the forwarded ref in the `setAndForwardRef` call. * Flow typed the final output (ForwardedScrollView) as an abstract component that takes in the props of the `ScrollView` class, and has all methods of both the inner host component (`measure`, `measureLayout`, etc) and the public methods (`scrollTo`, etc). Changes to mockScrollView: * Changed mockScrollView to be able to mock the function component instead of a class component * Updated necessary tests Changelog: [General] [Changed] - Make ScrollView use forwardRef Reviewed By: TheSavior Differential Revision: D19304480 fbshipit-source-id: 6c359897526d9d5ac6bc6ab6d5f9d82bfc0d8af4 --- Libraries/Components/ScrollView/ScrollView.js | 158 ++++++++++++++---- .../ScrollView/__mocks__/ScrollViewMock.js | 39 ----- .../ScrollView/__tests__/ScrollView-test.js | 17 +- .../__snapshots__/ScrollView-test.js.snap | 9 +- Libraries/Lists/FlatList.js | 15 +- Libraries/Lists/VirtualizedList.js | 5 +- .../Lists/__tests__/VirtualizedList-test.js | 2 +- .../LogBoxInspectorCodeFrame-test.js.snap | 8 +- jest/mockScrollView.js | 35 ++++ jest/setup.js | 25 ++- 10 files changed, 211 insertions(+), 102 deletions(-) delete mode 100644 Libraries/Components/ScrollView/__mocks__/ScrollViewMock.js create mode 100644 jest/mockScrollView.js diff --git a/Libraries/Components/ScrollView/ScrollView.js b/Libraries/Components/ScrollView/ScrollView.js index dd4a099430..c85d9c072a 100644 --- a/Libraries/Components/ScrollView/ScrollView.js +++ b/Libraries/Components/ScrollView/ScrollView.js @@ -62,20 +62,29 @@ if (Platform.OS === 'android') { RCTScrollContentView = ScrollContentViewNativeComponent; } -export type ScrollResponderType = { - // We'd like to do ...ScrollView here, however Flow doesn't seem - // to see the imperative methods of ScrollView that way. Workaround the - // issue by specifying them manually. +// Public methods for ScrollView +export type ScrollViewImperativeMethods = $ReadOnly<{| + getScrollResponder: $PropertyType, getScrollableNode: $PropertyType, getInnerViewNode: $PropertyType, getInnerViewRef: $PropertyType, getNativeScrollRef: $PropertyType, - setNativeProps: $PropertyType, scrollTo: $PropertyType, + scrollToEnd: $PropertyType, flashScrollIndicators: $PropertyType, - ...typeof ScrollResponder.Mixin, - ... -}; + + // ScrollResponder.Mixin public methods + scrollResponderZoomTo: $PropertyType< + typeof ScrollResponder.Mixin, + 'scrollResponderZoomTo', + >, + scrollResponderScrollNativeHandleToKeyboard: $PropertyType< + typeof ScrollResponder.Mixin, + 'scrollResponderScrollNativeHandleToKeyboard', + >, +|}>; + +export type ScrollResponderType = ScrollViewImperativeMethods; type IOSProps = $ReadOnly<{| /** @@ -581,6 +590,14 @@ export type Props = $ReadOnly<{| * instead of calling `getInnerViewRef`. */ innerViewRef?: React.Ref, + /** + * A ref to the Native ScrollView component. This ref can be used to call + * all of ScrollView's public methods, in addition to native methods like + * measure, measureLayout, etc. + */ + scrollViewRef?: React.Ref< + typeof ScrollViewNativeComponent & ScrollViewImperativeMethods, + >, |}>; type State = {| @@ -603,11 +620,14 @@ function createScrollResponder( } type ContextType = {|horizontal: boolean|} | null; -const Context = React.createContext(null); +const Context: React.Context = React.createContext(null); const standardHorizontalContext: ContextType = Object.freeze({ horizontal: true, }); const standardVerticalContext: ContextType = Object.freeze({horizontal: false}); +type ScrollViewComponentStatics = $ReadOnly<{| + Context: typeof Context, +|}>; /** * Component that wraps platform ScrollView while providing @@ -750,9 +770,37 @@ class ScrollView extends React.Component { } } - setNativeProps(props: {[key: string]: mixed, ...}) { - this._scrollViewRef && this._scrollViewRef.setNativeProps(props); - } + _setNativeRef = setAndForwardRef({ + getForwardedRef: () => this.props.scrollViewRef, + setLocalRef: ref => { + this._scrollViewRef = ref; + + /* + This is a hack. Ideally we would forwardRef to the underlying + host component. However, since ScrollView has it's own methods that can be + called as well, if we used the standard forwardRef then these + methods wouldn't be accessible and thus be a breaking change. + + Therefore we edit ref to include ScrollView's public methods so that + they are callable from the ref. + */ + if (ref) { + ref.getScrollResponder = this.getScrollResponder; + ref.getScrollableNode = this.getScrollableNode; + ref.getInnerViewNode = this.getInnerViewNode; + ref.getInnerViewRef = this.getInnerViewRef; + ref.getNativeScrollRef = this.getNativeScrollRef; + ref.scrollTo = this.scrollTo; + ref.scrollToEnd = this.scrollToEnd; + ref.flashScrollIndicators = this.flashScrollIndicators; + + // $FlowFixMe - This method was manually bound from ScrollResponder.mixin + ref.scrollResponderZoomTo = this.scrollResponderZoomTo; + // $FlowFixMe - This method was manually bound from ScrollResponder.mixin + ref.scrollResponderScrollNativeHandleToKeyboard = this.scrollResponderScrollNativeHandleToKeyboard; + } + }, + }); /** * Returns a reference to the underlying scroll responder, which supports @@ -760,14 +808,26 @@ class ScrollView extends React.Component { * implement this method so that they can be composed while providing access * to the underlying scroll responder's methods. */ - getScrollResponder(): ScrollResponderType { + getScrollResponder: () => ScrollResponderType = () => { + if (__DEV__) { + console.warn( + '`getScrollResponder()` is deprecated. This will be removed in a future release. ' + + 'Use instead.', + ); + } // $FlowFixMe - overriding type to include ScrollResponder.Mixin return ((this: any): ScrollResponderType); - } + }; - getScrollableNode(): ?number { + getScrollableNode: () => ?number = () => { + if (__DEV__) { + console.warn( + '`getScrollableNode()` is deprecated. This will be removed in a future release. ' + + 'Use instead.', + ); + } return ReactNative.findNodeHandle(this._scrollViewRef); - } + }; getInnerViewNode(): ?number { console.warn( @@ -785,9 +845,15 @@ class ScrollView extends React.Component { return this._innerViewRef; } - getNativeScrollRef(): ?React.ElementRef> { + getNativeScrollRef: () => ?React.ElementRef> = () => { + if (__DEV__) { + console.warn( + '`getNativeScrollRef()` is deprecated. This will be removed in a future release. ' + + 'Use instead.', + ); + } return this._scrollViewRef; - } + }; /** * Scrolls to a given x, y offset, either immediately or with a smooth animation. @@ -800,7 +866,7 @@ class ScrollView extends React.Component { * the function also accepts separate arguments as an alternative to the options object. * This is deprecated due to ambiguity (y before x), and SHOULD NOT BE USED. */ - scrollTo( + scrollTo: ( options?: | { x?: number, @@ -811,7 +877,18 @@ class ScrollView extends React.Component { | number, deprecatedX?: number, deprecatedAnimated?: boolean, - ) { + ) => void = ( + options?: + | { + x?: number, + y?: number, + animated?: boolean, + ... + } + | number, + deprecatedX?: number, + deprecatedAnimated?: boolean, + ) => { let x, y, animated; if (typeof options === 'number') { console.warn( @@ -831,7 +908,7 @@ class ScrollView extends React.Component { y: y || 0, animated: animated !== false, }); - } + }; /** * If this is a vertical ScrollView scrolls to the bottom. @@ -841,22 +918,24 @@ class ScrollView extends React.Component { * `scrollToEnd({animated: false})` for immediate scrolling. * If no options are passed, `animated` defaults to true. */ - scrollToEnd(options?: ?{animated?: boolean, ...}) { + scrollToEnd: (options?: ?{animated?: boolean, ...}) => void = ( + options?: ?{animated?: boolean, ...}, + ) => { // Default to true const animated = (options && options.animated) !== false; this._scrollResponder.scrollResponderScrollToEnd({ animated: animated, }); - } + }; /** * Displays the scroll indicators momentarily. * * @platform ios */ - flashScrollIndicators() { + flashScrollIndicators: () => void = () => { this._scrollResponder.scrollResponderFlashScrollIndicators(); - } + }; _getKeyForIndex(index, childArray) { const child = childArray[index]; @@ -959,9 +1038,6 @@ class ScrollView extends React.Component { }; _scrollViewRef: ?React.ElementRef> = null; - _setScrollViewRef = (ref: ?React.ElementRef>) => { - this._scrollViewRef = ref; - }; _innerViewRef: ?React.ElementRef = null; _setInnerViewRef = setAndForwardRef({ @@ -1182,7 +1258,7 @@ class ScrollView extends React.Component { /* $FlowFixMe(>=0.117.0 site=react_native_fb) This comment suppresses * an error found when Flow v0.117 was deployed. To see the error, * delete this comment and run Flow. */ - + {Platform.isTV ? null : refreshControl} {contentContainer} @@ -1200,14 +1276,14 @@ class ScrollView extends React.Component { + ref={this._setNativeRef}> {contentContainer} , ); } } return ( - + {contentContainer} ); @@ -1232,4 +1308,22 @@ const styles = StyleSheet.create({ }, }); -module.exports = ScrollView; +function Wrapper(props, ref) { + return ; +} +Wrapper.displayName = 'ScrollView'; +const ForwardedScrollView = React.forwardRef(Wrapper); + +// $FlowFixMe Add static context to ForwardedScrollView +ForwardedScrollView.Context = Context; + +ForwardedScrollView.displayName = 'ScrollView'; + +module.exports = ((ForwardedScrollView: $FlowFixMe): React.AbstractComponent< + React.ElementConfig, + $ReadOnly<{| + ...$Exact>>, + ...ScrollViewImperativeMethods, + |}>, +> & + ScrollViewComponentStatics); diff --git a/Libraries/Components/ScrollView/__mocks__/ScrollViewMock.js b/Libraries/Components/ScrollView/__mocks__/ScrollViewMock.js deleted file mode 100644 index 30e8cee004..0000000000 --- a/Libraries/Components/ScrollView/__mocks__/ScrollViewMock.js +++ /dev/null @@ -1,39 +0,0 @@ -/** - * 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. - * - * @format - * @flow strict-local - */ - -/* eslint-env jest */ - -'use strict'; - -const React = require('react'); -const View = require('../../View/View'); - -const requireNativeComponent = require('../../../ReactNative/requireNativeComponent'); - -import type {HostComponent} from '../../../Renderer/shims/ReactNativeTypes'; - -const RCTScrollView: HostComponent = requireNativeComponent( - 'RCTScrollView', -); - -const ScrollViewComponent: $FlowFixMe = jest.genMockFromModule('../ScrollView'); - -class ScrollViewMock extends ScrollViewComponent { - render(): React.Element { - return ( - - {this.props.refreshControl} - {this.props.children} - - ); - } -} - -module.exports = ScrollViewMock; diff --git a/Libraries/Components/ScrollView/__tests__/ScrollView-test.js b/Libraries/Components/ScrollView/__tests__/ScrollView-test.js index 5f048f7b33..fb7aa74a55 100644 --- a/Libraries/Components/ScrollView/__tests__/ScrollView-test.js +++ b/Libraries/Components/ScrollView/__tests__/ScrollView-test.js @@ -6,7 +6,7 @@ * * @format * @emails oncall+react_native - * @flow strict-local + * @flow-strict */ 'use strict'; @@ -14,6 +14,7 @@ const React = require('react'); const ScrollView = require('../ScrollView'); const ReactNativeTestTools = require('../../../Utilities/ReactNativeTestTools'); +const ReactTestRenderer = require('react-test-renderer'); const View = require('../../View/View'); const Text = require('../../../Text/Text'); @@ -33,4 +34,18 @@ describe('', () => { }, ); }); + it('should mock native methods and instance methods when mocked', () => { + jest.resetModules(); + jest.mock('../ScrollView'); + const ref = React.createRef(); + + ReactTestRenderer.create(); + + expect(ref.current != null && ref.current.measure).toBeInstanceOf( + jest.fn().constructor, + ); + expect(ref.current != null && ref.current.scrollTo).toBeInstanceOf( + jest.fn().constructor, + ); + }); }); diff --git a/Libraries/Components/ScrollView/__tests__/__snapshots__/ScrollView-test.js.snap b/Libraries/Components/ScrollView/__tests__/__snapshots__/ScrollView-test.js.snap index 7de0741031..4828ae59bb 100644 --- a/Libraries/Components/ScrollView/__tests__/__snapshots__/ScrollView-test.js.snap +++ b/Libraries/Components/ScrollView/__tests__/__snapshots__/ScrollView-test.js.snap @@ -36,6 +36,7 @@ exports[` should render as expected: should deep render when not m onTouchStart={[Function]} pagingEnabled={false} scrollBarThumbImage={null} + scrollViewRef={null} sendMomentumEvents={false} snapToEnd={true} snapToStart={true} @@ -70,21 +71,21 @@ exports[` should render as expected: should deep render when not m `; exports[` should render as expected: should shallow render as when mocked 1`] = ` - + Hello World! - + `; exports[` should render as expected: should shallow render as when not mocked 1`] = ` - + Hello World! - + `; diff --git a/Libraries/Lists/FlatList.js b/Libraries/Lists/FlatList.js index fac0114495..6c268ddeba 100644 --- a/Libraries/Lists/FlatList.js +++ b/Libraries/Lists/FlatList.js @@ -19,10 +19,8 @@ const StyleSheet = require('../StyleSheet/StyleSheet'); const invariant = require('invariant'); -import ScrollView, { - type ScrollResponderType, -} from '../Components/ScrollView/ScrollView'; -import type {ScrollViewNativeComponentType} from '../Components/ScrollView/ScrollViewNativeComponentType'; +import {type ScrollResponderType} from '../Components/ScrollView/ScrollView'; +import type {ScrollViewNativeComponentType} from '../Components/ScrollView/ScrollViewNativeComponentType.js'; import type {ViewStyleProp} from '../StyleSheet/StyleSheet'; import type { ViewToken, @@ -377,14 +375,7 @@ class FlatList extends React.PureComponent, void> { | ?React.ElementRef | ?React.ElementRef { if (this._listRef) { - const scrollRef = this._listRef.getScrollRef(); - if (scrollRef != null) { - if (scrollRef instanceof ScrollView) { - return scrollRef.getNativeScrollRef(); - } else { - return scrollRef; - } - } + return this._listRef.getScrollRef(); } } diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 2fcf4b36d7..4486b49060 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -1397,10 +1397,7 @@ class VirtualizedList extends React.PureComponent { // We are assuming that getOutermostParentListRef().getScrollRef() // is a non-null reference to a ScrollView this._scrollRef.measureLayout( - this.context.virtualizedList - .getOutermostParentListRef() - .getScrollRef() - .getNativeScrollRef(), + this.context.virtualizedList.getOutermostParentListRef().getScrollRef(), (x, y, width, height) => { this._offsetFromParentVirtualizedList = this._selectOffset({x, y}); this._scrollMetrics.contentLength = this._selectLength({ diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index d5ff7b4684..e99215a2d8 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -300,7 +300,7 @@ describe('VirtualizedList', () => { // This is checking if the ref acts like a ScrollView. If we had an // `isScrollView(ref)` method, that would be preferred. - expect(scrollRef.scrollTo).toBeInstanceOf(Function); + expect(scrollRef.scrollTo).toBeInstanceOf(jest.fn().constructor); }); it('getScrollRef for case where it returns a View', () => { diff --git a/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorCodeFrame-test.js.snap b/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorCodeFrame-test.js.snap index 6fc9fda0cf..c847ccd087 100644 --- a/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorCodeFrame-test.js.snap +++ b/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorCodeFrame-test.js.snap @@ -25,7 +25,7 @@ exports[`LogBoxInspectorCodeFrame should render a code frame 1`] = ` } } > - - + - - + { + return ( + + {this.props.refreshControl} + {this.props.children} + + ); + } + } + return ScrollViewMock; +} + +module.exports = (mockScrollView: $FlowFixMe); diff --git a/jest/setup.js b/jest/setup.js index c38003739d..5f85d8602f 100644 --- a/jest/setup.js +++ b/jest/setup.js @@ -126,11 +126,26 @@ jest '../Libraries/Components/RefreshControl/__mocks__/RefreshControlMock', ), ) - .mock('../Libraries/Components/ScrollView/ScrollView', () => - jest.requireActual( - '../Libraries/Components/ScrollView/__mocks__/ScrollViewMock', - ), - ) + .mock('../Libraries/Components/ScrollView/ScrollView', () => { + const baseComponent = mockComponent( + '../Libraries/Components/ScrollView/ScrollView', + { + ...MockNativeMethods, + getScrollResponder: jest.fn(), + getScrollableNode: jest.fn(), + getInnerViewNode: jest.fn(), + getInnerViewRef: jest.fn(), + getNativeScrollRef: jest.fn(), + scrollTo: jest.fn(), + scrollToEnd: jest.fn(), + flashScrollIndicators: jest.fn(), + scrollResponderZoomTo: jest.fn(), + scrollResponderScrollNativeHandleToKeyboard: jest.fn(), + }, + ); + const mockScrollView = jest.requireActual('./mockScrollView'); + return mockScrollView(baseComponent); + }) .mock('../Libraries/Components/ActivityIndicator/ActivityIndicator', () => mockComponent( '../Libraries/Components/ActivityIndicator/ActivityIndicator',