Skip to content

Commit

Permalink
Make ScrollView use ForwardRef
Browse files Browse the repository at this point in the history
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 `<ScrollView ref={myRef} />` and directly call all methods of ScrollView and host components on `myRef`.

Previous usage:
```
const myRef = React.createRef<React.ElementRef<typeof ScrollView>>();
<ScrollView ref={myRef} />

const innerViewRef = myRef.current.getNativeScrollRef();

innerViewRef.measure();
```
New usage:
```
const myRef = React.createRef<React.ElementRef<typeof View>>();
<ScrollView ref={myRef} />

// 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
  • Loading branch information
kacieb authored and facebook-github-bot committed Mar 26, 2020
1 parent 93ee5b2 commit d2f314a
Show file tree
Hide file tree
Showing 10 changed files with 211 additions and 102 deletions.
158 changes: 126 additions & 32 deletions Libraries/Components/ScrollView/ScrollView.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<ScrollView, 'getScrollResponder'>,
getScrollableNode: $PropertyType<ScrollView, 'getScrollableNode'>,
getInnerViewNode: $PropertyType<ScrollView, 'getInnerViewNode'>,
getInnerViewRef: $PropertyType<ScrollView, 'getInnerViewRef'>,
getNativeScrollRef: $PropertyType<ScrollView, 'getNativeScrollRef'>,
setNativeProps: $PropertyType<ScrollView, 'setNativeProps'>,
scrollTo: $PropertyType<ScrollView, 'scrollTo'>,
scrollToEnd: $PropertyType<ScrollView, 'scrollToEnd'>,
flashScrollIndicators: $PropertyType<ScrollView, 'flashScrollIndicators'>,
...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<{|
/**
Expand Down Expand Up @@ -581,6 +590,14 @@ export type Props = $ReadOnly<{|
* instead of calling `getInnerViewRef`.
*/
innerViewRef?: React.Ref<typeof View>,
/**
* 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 = {|
Expand All @@ -603,11 +620,14 @@ function createScrollResponder(
}

type ContextType = {|horizontal: boolean|} | null;
const Context = React.createContext<ContextType>(null);
const Context: React.Context<ContextType> = 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
Expand Down Expand Up @@ -750,24 +770,64 @@ class ScrollView extends React.Component<Props, State> {
}
}

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
* operations like `scrollTo`. All ScrollView-like components should
* 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 <ScrollView ref={myRef} /> 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 <ScrollView ref={myRef} /> instead.',
);
}
return ReactNative.findNodeHandle(this._scrollViewRef);
}
};

getInnerViewNode(): ?number {
console.warn(
Expand All @@ -785,9 +845,15 @@ class ScrollView extends React.Component<Props, State> {
return this._innerViewRef;
}

getNativeScrollRef(): ?React.ElementRef<HostComponent<mixed>> {
getNativeScrollRef: () => ?React.ElementRef<HostComponent<mixed>> = () => {
if (__DEV__) {
console.warn(
'`getNativeScrollRef()` is deprecated. This will be removed in a future release. ' +
'Use <ScrollView ref={myRef} /> instead.',
);
}
return this._scrollViewRef;
}
};

/**
* Scrolls to a given x, y offset, either immediately or with a smooth animation.
Expand All @@ -800,7 +866,7 @@ class ScrollView extends React.Component<Props, State> {
* 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,
Expand All @@ -811,7 +877,18 @@ class ScrollView extends React.Component<Props, State> {
| 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(
Expand All @@ -831,7 +908,7 @@ class ScrollView extends React.Component<Props, State> {
y: y || 0,
animated: animated !== false,
});
}
};

/**
* If this is a vertical ScrollView scrolls to the bottom.
Expand All @@ -841,22 +918,24 @@ class ScrollView extends React.Component<Props, State> {
* `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];
Expand Down Expand Up @@ -959,9 +1038,6 @@ class ScrollView extends React.Component<Props, State> {
};

_scrollViewRef: ?React.ElementRef<HostComponent<mixed>> = null;
_setScrollViewRef = (ref: ?React.ElementRef<HostComponent<mixed>>) => {
this._scrollViewRef = ref;
};

_innerViewRef: ?React.ElementRef<typeof View> = null;
_setInnerViewRef = setAndForwardRef({
Expand Down Expand Up @@ -1182,7 +1258,7 @@ class ScrollView extends React.Component<Props, State> {
/* $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. */
<ScrollViewClass {...props} ref={this._setScrollViewRef}>
<ScrollViewClass {...props} ref={this._setNativeRef}>
{Platform.isTV ? null : refreshControl}
{contentContainer}
</ScrollViewClass>
Expand All @@ -1200,14 +1276,14 @@ class ScrollView extends React.Component<Props, State> {
<ScrollViewClass
{...props}
style={[baseStyle, inner]}
ref={this._setScrollViewRef}>
ref={this._setNativeRef}>
{contentContainer}
</ScrollViewClass>,
);
}
}
return (
<ScrollViewClass {...props} ref={this._setScrollViewRef}>
<ScrollViewClass {...props} ref={this._setNativeRef}>
{contentContainer}
</ScrollViewClass>
);
Expand All @@ -1232,4 +1308,22 @@ const styles = StyleSheet.create({
},
});

module.exports = ScrollView;
function Wrapper(props, ref) {
return <ScrollView {...props} scrollViewRef={ref} />;
}
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<typeof ScrollView>,
$ReadOnly<{|
...$Exact<React.ElementRef<HostComponent<mixed>>>,
...ScrollViewImperativeMethods,
|}>,
> &
ScrollViewComponentStatics);
39 changes: 0 additions & 39 deletions Libraries/Components/ScrollView/__mocks__/ScrollViewMock.js

This file was deleted.

17 changes: 16 additions & 1 deletion Libraries/Components/ScrollView/__tests__/ScrollView-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
*
* @format
* @emails oncall+react_native
* @flow strict-local
* @flow-strict
*/

'use strict';

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');

Expand All @@ -33,4 +34,18 @@ describe('<ScrollView />', () => {
},
);
});
it('should mock native methods and instance methods when mocked', () => {
jest.resetModules();
jest.mock('../ScrollView');
const ref = React.createRef();

ReactTestRenderer.create(<ScrollView ref={ref} />);

expect(ref.current != null && ref.current.measure).toBeInstanceOf(
jest.fn().constructor,
);
expect(ref.current != null && ref.current.scrollTo).toBeInstanceOf(
jest.fn().constructor,
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ exports[`<ScrollView /> should render as expected: should deep render when not m
onTouchStart={[Function]}
pagingEnabled={false}
scrollBarThumbImage={null}
scrollViewRef={null}
sendMomentumEvents={false}
snapToEnd={true}
snapToStart={true}
Expand Down Expand Up @@ -70,21 +71,21 @@ exports[`<ScrollView /> should render as expected: should deep render when not m
`;

exports[`<ScrollView /> should render as expected: should shallow render as <ScrollView /> when mocked 1`] = `
<ScrollView>
<ForwardRef(ScrollView)>
<View>
<Text>
Hello World!
</Text>
</View>
</ScrollView>
</ForwardRef(ScrollView)>
`;

exports[`<ScrollView /> should render as expected: should shallow render as <ScrollView /> when not mocked 1`] = `
<ScrollView>
<ForwardRef(ScrollView)>
<View>
<Text>
Hello World!
</Text>
</View>
</ScrollView>
</ForwardRef(ScrollView)>
`;
Loading

0 comments on commit d2f314a

Please sign in to comment.