MentionWarnings [nfc]: Convert to function component.

[chrisbobbe]

For annotating the default export of MentionWarnings, for #4907, I think it actually may be easier to first convert it to a function component and remove the connect call. This is a little surprising because it had been on my list of components that I thought would be pretty tricky to convert.

I didn't find a way to get thorough type-checking of MentionWarnings's methods that ComposeBox was calling on a ref, until I converted MentionWarnings to a function component. After this PR, we should get that type-checking, without having to manually repeat any types.

I tested the component on my iPhone and it worked as I expected it to.

[chrisbobbe]

Unfortunately, none of these three worked (see errors alongside):

• React.createRef<React$ElementRef<MentionWarnings>>()

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/compose/ComposeBox.js:142:54

Cannot use MentionWarnings as a type. A name can be used as a type only if it refers to a type, interface, class, or
enum definition. To get the type of a non-class value, use typeof. [value-as-type]

     139│   topicInputRef = React.createRef<$FlowFixMe>();
     140│
     141│   // Here, Flow seems to infer the correct type based on usage.
     142│   mentionWarnings = React.createRef<React$ElementRef<MentionWarnings>>();
     143│
     144│   inputBlurTimeoutId: ?TimeoutID = null;
     145│

• React.createRef<MentionWarnings>() (from Upgrade react-native-webview to latest, 11.6.4, on the path to the RN 64 upgrade. #4843 (comment))

Cannot use MentionWarnings as a type. A name can be used as a type only if it refers to a type, interface, class, or
enum definition. To get the type of a non-class value, use typeof. [value-as-type]

     139│   topicInputRef = React.createRef<$FlowFixMe>();
     140│
     141│   // Here, Flow seems to infer the correct type based on usage.
     142│   mentionWarnings = React.createRef<MentionWarnings>();
     143│
     144│   inputBlurTimeoutId: ?TimeoutID = null;
     145│

• React.createRef<typeof MentionWarnings>()

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/compose/ComposeBox.js:304:37

Cannot call this.mentionWarnings.current?.handleMentionSubscribedCheck because property handleMentionSubscribedCheck is
missing in AbstractComponent [1]. [incompatible-use]

 [1] 142│   mentionWarnings = React.createRef<typeof MentionWarnings>();
        :
     301│     if (lastWordPrefix === '@') {
     302│       // https://github.com/eslint/eslint/issues/11045
     303│       // eslint-disable-next-line no-unused-expressions
     304│       this.mentionWarnings.current?.handleMentionSubscribedCheck(completion);
     305│     }
     306│   };
     307│


Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/compose/ComposeBox.js:382:36

Cannot call this.mentionWarnings.current.clearMentionWarnings because property clearMentionWarnings is missing in
AbstractComponent [1]. [incompatible-use]

 [1] 142│   mentionWarnings = React.createRef<typeof MentionWarnings>();
        :
     379│     this.setMessageInputValue('');
     380│
     381│     if (this.mentionWarnings.current) {
     382│       this.mentionWarnings.current.clearMentionWarnings();
     383│     }
     384│
     385│     dispatch(sendTypingStop(narrow));


Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/compose/ComposeBox.js:500:63

Cannot create MentionWarnings element because in property ref: [incompatible-type]
 • Either object type [1] is incompatible with AbstractComponent [2] in property current.
 • Or a call signature declaring the expected parameter / return type is missing in object type [3] but exists in
   function type [4].

     src/compose/ComposeBox.js
 [2] 142│   mentionWarnings = React.createRef<typeof MentionWarnings>();
        :
     497│
     498│     return (
     499│       <View style={this.styles.wrapper}>
     500│         <MentionWarnings narrow={narrow} stream={stream} ref={this.mentionWarnings} />
     501│         <View style={[this.styles.autocompleteWrapper, { marginBottom: height }]}>
     502│           <TopicAutocomplete
     503│             isFocused={isTopicFocused}

     /private/tmp/flow/flowlib_38b4cf0c/react.js
 [4] 197│   | ((React$ElementRef<ElementType> | null) => mixed)
        :
 [3] 248│   ): {|current: null | T|};

     src/compose/MentionWarnings.js
 [1] 170│ const MentionWarnings: AbstractComponent<Props, ImperativeHandle> = forwardRef<



Found 3 errors

---

I found that the following would work, but I would appreciate hearing thoughts about it 🙂. I haven't yet been able to reconcile it with our findings in that linked discussion.

React.createRef<React$ElementRef<typeof MentionWarnings>>()

[gnprice]

React.createRef<React$ElementRef<typeof MentionWarnings>>()

Yeah, that looks correct to me.

In that discussion I wrote this:

Hmm, actually, now that I've reasoned through it more fully like that, I see that if my understanding of what's going on here is right, then React$ElementRef<typeof Foo> -- for any Foo we'll ever write -- is actually just a long way of spelling Foo.

But that was based in part on this, which I now see is… incomplete!

Hmm, I guess ref can't be used on function components anyway.

What I really meant was that for a Foo that is a class component-type, React$ElementRef<typeof Foo> is actually just a long way of spelling Foo.

As I now understand it, it is indeed true that ref can't be used directly on a function component-type… but there are a bunch of other flavors of component type besides functions and classes, in particular the return values of forwardRef.

More generally, I think the story is that React$ElementRef takes anything that looks like React$AbstractComponent<Config, Instance>, and it picks out that Instance type. Here, we say right in the type annotation (new in this branch):

const MentionWarnings: AbstractComponent<Props, ImperativeHandle> = // …

so typeof MentionWarnings is AbstractComponent<Props, ImperativeHandle>;
so React$ElementRef<typeof MentionWarnings> is ImperativeHandle. Which is the type you want here, and the same as the type that Flow infers for it given just React.createRef() with no type parameter. (It's using the fact that we pass this as the ref pseudoprop to MentionWarnings, down below.)

I started writing down some more notes on what I've worked out from studying these core-React types, and ended up expanding it into a new page in our docs tree:
https://github.com/zulip/zulip-mobile/blob/470ad571782501039369963eaf8d262802a2f07b/docs/background/react.md

See also where the Flow upstream docs describe ElementRef, in a page I actually only just now found after writing that up. It doesn't cover what happens with forwardRef. But I think it does explain what the major motivation for ElementRef was, given that its behavior on class component-types and function component-types is so boring: it's for DOM element-types, where it maps 'div' to HTMLDivElement and 'input' to HTMLInputElement and so on.

[gnprice]

Anyway, given that Flow successfully infers the type here just fine, and because spelling it out is kind of a mouthful, I'm happy to just leave it to be inferred.

[chrisbobbe]

Thanks for all that detail and the new doc! 😄

Anyway, given that Flow successfully infers the type here just fine, and because spelling it out is kind of a mouthful, I'm happy to just leave it to be inferred.

Now that I'm confident about how to spell it out, I think it might be better to do so. It seems to help Flow give better error messages.

If I do

- this.mentionWarnings.current.clearMentionWarnings();
+ this.mentionWarnings.current.clearMentionWarnings(123);

without spelling it out, then I get this error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/compose/ComposeBox.js:500:63

Cannot create MentionWarnings element because a call signature declaring the expected parameter / return type is missing
in object type [1] but exists in function type [2] in property ref. [incompatible-type]

     src/compose/ComposeBox.js
     497│
     498│     return (
     499│       <View style={this.styles.wrapper}>
     500│         <MentionWarnings narrow={narrow} stream={stream} ref={this.mentionWarnings} />
     501│         <View style={[this.styles.autocompleteWrapper, { marginBottom: height }]}>
     502│           <TopicAutocomplete
     503│             isFocused={isTopicFocused}

     /private/tmp/flow/flowlib_3f0b09e6/react.js
 [2] 197│   | ((React$ElementRef<ElementType> | null) => mixed)
        :
 [1] 248│   ): {|current: null | T|};

But if I do that with spelling it out, I get this error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/compose/ComposeBox.js:382:57

Cannot call this.mentionWarnings.current.clearMentionWarnings because no arguments are expected by function type [1].
[extra-arg]

     src/compose/ComposeBox.js
     379│     this.setMessageInputValue('');
     380│
     381│     if (this.mentionWarnings.current) {
     382│       this.mentionWarnings.current.clearMentionWarnings(123);
     383│     }
     384│
     385│     dispatch(sendTypingStop(narrow));

     src/compose/MentionWarnings.js
 [1]  37│   clearMentionWarnings(): void,

Which is more specific and nicer. 🙂

[gnprice]

Hmm indeed -- good thought to check that!

That's a good reason to write down the annotation.

[gnprice]

Thanks @chrisbobbe ! Comments below. One is long, but the length is all background; all are simple to handle.

[gnprice]

React.createRef<React$ElementRef<typeof MentionWarnings>>()

Yeah, that looks correct to me.

In that discussion I wrote this:

Hmm, actually, now that I've reasoned through it more fully like that, I see that if my understanding of what's going on here is right, then React$ElementRef<typeof Foo> -- for any Foo we'll ever write -- is actually just a long way of spelling Foo.

But that was based in part on this, which I now see is… incomplete!

Hmm, I guess ref can't be used on function components anyway.

What I really meant was that for a Foo that is a class component-type, React$ElementRef<typeof Foo> is actually just a long way of spelling Foo.

As I now understand it, it is indeed true that ref can't be used directly on a function component-type… but there are a bunch of other flavors of component type besides functions and classes, in particular the return values of forwardRef.

More generally, I think the story is that React$ElementRef takes anything that looks like React$AbstractComponent<Config, Instance>, and it picks out that Instance type. Here, we say right in the type annotation (new in this branch):

const MentionWarnings: AbstractComponent<Props, ImperativeHandle> = // …

so typeof MentionWarnings is AbstractComponent<Props, ImperativeHandle>;
so React$ElementRef<typeof MentionWarnings> is ImperativeHandle. Which is the type you want here, and the same as the type that Flow infers for it given just React.createRef() with no type parameter. (It's using the fact that we pass this as the ref pseudoprop to MentionWarnings, down below.)

I started writing down some more notes on what I've worked out from studying these core-React types, and ended up expanding it into a new page in our docs tree:
https://github.com/zulip/zulip-mobile/blob/470ad571782501039369963eaf8d262802a2f07b/docs/background/react.md

See also where the Flow upstream docs describe ElementRef, in a page I actually only just now found after writing that up. It doesn't cover what happens with forwardRef. But I think it does explain what the major motivation for ElementRef was, given that its behavior on class component-types and function component-types is so boring: it's for DOM element-types, where it maps 'div' to HTMLDivElement and 'input' to HTMLInputElement and so on.

[gnprice]

Anyway, given that Flow successfully infers the type here just fine, and because spelling it out is kind of a mouthful, I'm happy to just leave it to be inferred.

[chrisbobbe]

Thanks for the review and that doc! Revision pushed.

[gnprice]

Thanks! Merged.