[createReactClass] DrawerLayoutAndroid: Convert to ES6 class

[empyrical]

This pull requests converts DrawerLayoutAndroid from a createReactClass-based component to an ES6 class component, removing its use of NativeMethodsMixin and prop-types in the process.

Unfortunately this couldn't be moved over to a full forwardRef to the native component, because it has the methods openDrawer and closeDrawer as part of its API, and they can't be called directly on the native component. A createRef-based ref to the native component can be accessed from the nativeRef property of a DrawerLayoutAndroid instance if a native ref is needed.

The Flow types for callbacks have also been filled out.

Test Plan:

yarn flow-check-android passes. I've tried it out in the Android simulator, and no regressions have been noted.

Release Notes:

[GENERAL] [ENHANCEMENT] [Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js] - Converted to class component

[empyrical]

Curious about thoughts on this. Is it left over from an old RN version? Not sure if it is safe to remove.

[elicwhite]

There are no internal callsites of this. The only other thing that defines getInnerViewNode is ScrollView and innerViewRef is never ScrollView. Let's kill it.

[RSNara]

🤔This refactor is incomplete.

Did you consider doing 👇 to implement the NativeMethodsMixin methods?

class DrawerLayoutAndroid extends React.Component<Props, State> {
  // ...
  blur(): void {
    return this.nativeRef.blur();
  }

  focus(): void {
    return this.nativeRef.focus();
  }

  measure(callback: MeasureOnSuccessCallback): void {
    return this.nativeRef.measure(callback);
  }

  measureInWindow(callback: MeasureInWindowOnSuccessCallback): void {
    return this.nativeRef.measureInWindow(callback);
  }

  measureLayout(
    relativeToNativeNode: number,
    onSuccess: MeasureLayoutOnSuccessCallback,
    onFail: () => void,
  ): void {
    return this.nativeRef.measureLayout(
      relativeToNativeNode, 
      onSuccess,
      onFail,
    );
  }

  setNativeProps(nativeProps: Object): void {
    return this.nativeRef.setNativeProps(nativeProps);
  }

  // ...
}

[empyrical]

My thoughts on why I did it that way: That would make the wrapper more complex, and would have to be changed whenever NativeComponent is changed. Making the native ref a non-private (_underscore-prefixed) property of the class would still give user code access to the native component if need be. (This would need to be documented!)

This is the approach WebView took:

https://github.com/react-native-community/react-native-webview/blob/7f3534463291893154bd41dc560c092a7b9bf746/js/WebView.ios.js#L143

Can, of course, update this if this is not a good option however! 👍

[elicwhite]

It seems like this is a problem with needing to add additional methods to the public API of the component when using forwardRef.

As an additional data point, of the 8 or so callsites of this component inside of Facebook, they only use openDrawer and closeDrawer and none of them access the other methods from NativeMethodsMixin.

If we want to keep the API the same, I think I would bias towards what @RSNara proposed so that we can hide the implementation details. Otherwise maybe we should just drop support for the native methods?

[empyrical]

Implemented the native methods. Could also serve as some boilerplate for if this needs to be done on other classes in the future, too.

How do you feel about this implementation?

react-native/Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js

Lines 321 to 354 in 2d97b41

	/**
	* Native methods
	*/
	blur() {
	nullthrows(this._nativeRef.current).blur();
	}
	focus() {
	nullthrows(this._nativeRef.current).focus();
	}
	measure(callback: MeasureOnSuccessCallback) {
	nullthrows(this._nativeRef.current).measure(callback);
	}
	measureInWindow(callback: MeasureInWindowOnSuccessCallback) {
	nullthrows(this._nativeRef.current).measureInWindow(callback);
	}
	measureLayout(
	relativeToNativeNode: number,
	onSuccess: MeasureLayoutOnSuccessCallback,
	onFail?: () => void,
	) {
	nullthrows(this._nativeRef.current).measureLayout(
	relativeToNativeNode,
	onSuccess,
	onFail,
	);
	}
	setNativeProps(nativeProps: Object) {
	nullthrows(this._nativeRef.current).setNativeProps(nativeProps);
	}

Also looks like CI is failing because circleci is not yet able to see the latest metro version on npm

[RSNara]

Thanks for iterating on this, @empyrical!

[facebook-github-bot]

@RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

@empyrical merged commit bea3bb6 into facebook:master.