Skip to content

Commit

Permalink
types: Relocate some $FlowFixMes and correct their TODOs.
Browse files Browse the repository at this point in the history
As noted in the code comments, one mistake we were making (not seen
until now) was passing the wrong type parameter to `React$Ref`; we
should be passing a `React$ElementRef` type [1].

When we do that, the existing `$FlowFixMe`s (the ones with comments
about RN v0.63) are marked as unused, as we'd expect. But
unfortunately, at that point, `.current` is typed as
`any(implicit)`, which we definitely don't want. And that doesn't
seem to get fixed by upgrading to RN v0.63, so, remove the comments
about that new version. Greg suggests this may be caused by bugs in
Flow's special support for React [2].

So, mention our new knowledge that we need to be using
`React$ElementRef`, but retain some fixmes and comments alerting us
to the fact that `.current` is untyped and that we need to be
careful about using it.

[1] zulip#4278 (comment)
[2] zulip#4420 (comment)
  • Loading branch information
chrisbobbe authored and Gautam-Arora24 committed Feb 2, 2021
1 parent c9e0819 commit 0b1d351
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 26 deletions.
7 changes: 6 additions & 1 deletion src/common/Input.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ export type Props = $ReadOnly<{|
...React$ElementConfig<typeof TextInput>,
placeholder: LocalizableText,
onChangeText?: (text: string) => void,
textInputRef?: React$Ref<typeof TextInput>,

// We should replace the fixme with
// `React$ElementRef<typeof TextInput>` when we can. Currently, that
// would make `.current` be `any(implicit)`, which we don't want;
// this is probably down to bugs in Flow's special support for React.
textInputRef?: React$Ref<$FlowFixMe>,

_: GetText,
|}>;
Expand Down
12 changes: 7 additions & 5 deletions src/common/InputWithClearButton.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { View, TextInput } from 'react-native';
import { View } from 'react-native';

import Input from './Input';
import type { Props as InputProps } from './Input';
Expand Down Expand Up @@ -32,7 +32,11 @@ export default class InputWithClearButton extends PureComponent<Props, State> {
canBeCleared: false,
text: '',
};
textInputRef = React.createRef<typeof TextInput>();
// We should replace the fixme with
// `React$ElementRef<typeof TextInput>` when we can. Currently, that
// would make `.current` be `any(implicit)`, which we don't want;
// this is probably down to bugs in Flow's special support for React.
textInputRef = React.createRef<$FlowFixMe>();

handleChangeText = (text: string) => {
this.setState({
Expand All @@ -47,9 +51,7 @@ export default class InputWithClearButton extends PureComponent<Props, State> {
handleClear = () => {
this.handleChangeText('');
if (this.textInputRef.current) {
// Should be fixed in RN v0.63 (#4245); see
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351.
// $FlowFixMe
// `.current` is not type-checked; see definition.
this.textInputRef.current.clear();
}
};
Expand Down
18 changes: 10 additions & 8 deletions src/common/SmartUrlInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,19 @@ export default class SmartUrlInput extends PureComponent<Props, State> {
state = {
value: '',
};
textInputRef = React.createRef<typeof TextInput>();

// We should replace the fixme with
// `React$ElementRef<typeof TextInput>` when we can. Currently, that
// would make `.current` be `any(implicit)`, which we don't want;
// this is probably down to bugs in Flow's special support for React.
textInputRef = React.createRef<$FlowFixMe>();

unsubscribeFocusListener: () => void;

componentDidMount() {
this.unsubscribeFocusListener = this.props.navigation.addListener('focus', () => {
if (this.textInputRef.current) {
// Should be fixed in RN v0.63 (#4245); see
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351.
// $FlowFixMe
// `.current` is not type-checked; see definition.
this.textInputRef.current.focus();
}
});
Expand All @@ -99,13 +103,11 @@ export default class SmartUrlInput extends PureComponent<Props, State> {
urlPress = () => {
const { textInputRef } = this;
if (textInputRef.current) {
// Should be fixed in RN v0.63 (#4245); see
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351.
// $FlowFixMe
// `.current` is not type-checked; see definition.
textInputRef.current.blur();
setTimeout(() => {
if (textInputRef.current) {
// $FlowFixMe - same as above
// `.current` is not type-checked; see definition.
textInputRef.current.focus();
}
}, 100);
Expand Down
22 changes: 10 additions & 12 deletions src/compose/ComposeBox.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { Platform, View, TextInput, findNodeHandle } from 'react-native';
import { Platform, View, findNodeHandle } from 'react-native';
import type { LayoutEvent } from 'react-native/Libraries/Types/CoreEventTypes';
import TextInputReset from 'react-native-text-input-reset';
import { type EdgeInsets } from 'react-native-safe-area-context';
Expand Down Expand Up @@ -114,9 +114,7 @@ const updateTextInput = (textInput, text) => {
return;
}

// Should be fixed in RN v0.63 (#4245); see
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351.
// $FlowFixMe
// `textInput` is untyped; see definition.
textInput.setNativeProps({ text });

if (text.length === 0 && TextInputReset) {
Expand All @@ -130,8 +128,12 @@ class ComposeBox extends PureComponent<Props, State> {
static contextType = ThemeContext;
context: ThemeData;

messageInputRef = React.createRef<typeof TextInput>();
topicInputRef = React.createRef<typeof TextInput>();
// We should replace the fixme with
// `React$ElementRef<typeof TextInput>` when we can. Currently, that
// would make `.current` be `any(implicit)`, which we don't want;
// this is probably down to bugs in Flow's special support for React.
messageInputRef = React.createRef<$FlowFixMe>();
topicInputRef = React.createRef<$FlowFixMe>();

// TODO: Type-check this, once we've adjusted our `react-redux`
// wrapper to do the right thing. It should be
Expand Down Expand Up @@ -349,9 +351,7 @@ class ComposeBox extends PureComponent<Props, State> {
}
completeEditMessage();
if (this.messageInputRef.current !== null) {
// Should be fixed in RN v0.63 (#4245); see
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351.
// $FlowFixMe
// `.current` is not type-checked; see definition.
this.messageInputRef.current.blur();
}
};
Expand All @@ -366,9 +366,7 @@ class ComposeBox extends PureComponent<Props, State> {
this.setMessageInputValue(message);
this.setTopicInputValue(topic);
if (this.messageInputRef.current !== null) {
// Should be fixed in RN v0.63 (#4245); see
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351.
// $FlowFixMe
// `.current` is not type-checked; see definition.
this.messageInputRef.current.focus();
}
}
Expand Down

0 comments on commit 0b1d351

Please sign in to comment.