[WIP] Convert TextInput to class component

[empyrical]

Summary

This is attempt 2 at porting TextInput to an ES6 class. Last attempt needed to be reverted because of an internal error at FB - one that I unfortunately was not able to recreate locally in Debug or Release builds. I am hoping that things may have changed enough on FB's end that this is no longer an issue - but should keep an eye out for this when merging.

This is a WIP for two Flow-related reasons that I need input on.

The first is a confusing Flow error I encounter with the forwardRef - I am probably missing something, but I am not sure why it's not able to find the value for the template parameter Config even when I explicitly provide it to props

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ Libraries/Components/TextInput/TextInput.js:1248:26

Missing type annotation for Config. Config is a type parameter declared in function type [1] and was implicitly
instantiated at call of method forwardRef [2].

     Libraries/Components/TextInput/TextInput.js
     1245│   };
     1246│ }
     1247│
 [2] 1248│ const TextInputWithRef = React.forwardRef(function TextInputWithRef(
     1249│   props: Props,
     1250│   forwardedRef?: ?React.Ref<Class<TextInputType>>,
     1251│ ) {
     1252│   return <TextInput {...props} forwardedRef={forwardedRef} />;
     1253│ });
     1254│
     1255│ const styles = StyleSheet.create({
     1256│   multilineInput: {

     /private/tmp/flow/flowlib_2b0c5fea/react.js
 [1]  300│   declare export function forwardRef<Config, Instance>(
      301│     render: (
      302│       props: Config,
      303│       ref: { current: null | Instance, ... } | ((null | Instance) => mixed),
      304│     ) => React$Node,
      305│   ): React$AbstractComponent<Config, Instance>;

Secondly, now that props being passed down are properly seen by Flow, the usage of AndroidTextInputNativeComponent introduced in this commit have a great deal of Flow errors, because TextInput's props is ...spread onto it, and there are a great deal of props in TextInput not present in AndroidTextInputNativeComponent.

Should a $FlowFixMe be applied here for the time being? Not quite sure what should be done to reconcile this.

Changelog

[General] [Changed] - Converted TextInput to ES6 class-based component

Test Plan

I have tried this version of <TextInput /> out in RNTester (Debug and Release builds) and no regressions have been noted.

[JoshuaGross]

Is it possible somehow to restrict the props spread into AndroidTextInputNativeComponent to just the props that AndroidTextInputNativeComponent accepts, somehow? That seems like it might be desirable anyway?
If not, I'd be fine with $FlowFixMe, but only if we absolutely need to. Others might have differing opinions though.

[empyrical]

I think that just applying the needed props is probably the best way to go too! iOS (both the legacy and non-legacy rendering functions) will end up needing this treatment as well.

[JoshuaGross]

Just to clarify, @empyrical, are you saying that you will update the PR? Just checking in.

[empyrical]

Yep! I just haven't had the time yet. Sorry, should have been clearer

[empyrical]

There's some small Flow conflicts currently with the event types in TextInput and the event types in the Native Component, like contentSize not being $ReadOnly in the Native Component:

react-native/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js

Lines 324 to 330 in 543420c

	onContentSizeChange?: ?DirectEventHandler<
	$ReadOnly<{|
	target: Int32,
	// contentSize: $ReadOnly<{|width: Double, height: Double|}>,
	contentSize: {|width: Double, height: Double|},
	|}>,
	>,

While being readonly in TextInput:

react-native/Libraries/Components/TextInput/TextInput.js

Lines 77 to 85 in 543420c

	export type ContentSizeChangeEvent = SyntheticEvent<
	$ReadOnly<{|
	target: number,
	contentSize: $ReadOnly<{|
	width: number,
	height: number,
	|}>,
	|}>,
	>;

I notice that the native component has lines with contentSize being readonly commented out, suggesting perhaps that codegen might have had issues with it.

What should be the best way to handle this? Temporarily make TextInput's contentSize no longer readonly until the Native Component can have it made readonly?

[elicwhite]

Any update on this @empyrical? I'd love to get this in. We are starting to need that forwarded ref and I currently need to work around this not having forwardRef by having a getNativeRef function which is gross. 😀

[empyrical]

Hey! I think i might have time tomorrow to work on this again. A lot of time is being spent cleaning up flow errors, but functionality-wise in RNTester everything works great!

[cpojer]

Is there any chance we could add at least Jest tests for this component to verify the behavior of the JS is the same as before?

[elicwhite]

I talked with @cpojer in person. I definitely would love to see tests for TextInput but I don't want to block this change on those tests being written. I think tests for TextInput is a larger problem we need to tackle but this refactor to TextInput is needed for our Fabric work. @JoshuaGross and I will take responsibility for landing and following this through.

[facebook-github-bot]

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

[facebook-github-bot]

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

[empyrical]

Closing. Ended up in a commit but wasn't autoclosed 👍