flow: Annotate some more exported React components. #4914

chrisbobbe · 2021-07-21T17:57:00Z

Another bundle of these, after #4908, toward #4907.

In general, this turns out to be much easier to do for function components than class components, so most of this series is just doing those conversions. Then there's a commit at the end that adds the annotations.

Temporarily adding well_formed_exports=true shows that this series takes us from 205 down to 145 complaints of missing export annotations.

gnprice

Thanks @chrisbobbe ! Generally these look good; comments on some of them below.

src/users/UsersScreen.js

gnprice · 2021-07-23T23:28:39Z

src/animation/AnimatedComponent.js

+
+  const prevVisible = usePrevious(visible);
+
+  const animatedValue = useRef(new Animated.Value(0));


Hmm, a bit awkward that this means we're creating a new Animated.Value on each subsequent render and then throwing it away.

But I don't see a good way in the Hooks API to avoid that. Hopefully that constructor isn't expensive 🤷

Hmm, yeah, interesting...

See "You might also occasionally want to avoid re-creating the useRef() initial value" here. The suggested solution there looks a little awkward though.

Cool, yeah. That looks like something we'd want to do if we actually know the constructor is expensive, but not something we'd want to default to speculatively.

gnprice · 2021-07-23T23:30:19Z

src/animation/AnimatedComponent.js

+  const animate = useCallback(() => {
+    Animated.timing(animatedValue.current, {
+      toValue: visible ? fullValue : 0,
+      delay,
      duration: 300,
-      useNativeDriver: this.props.useNativeDriver,
+      useNativeDriver,
      easing: Easing.out(Easing.poly(4)),
    }).start();
-  }
+  }, [delay, fullValue, useNativeDriver, visible]);


Huh, I'm surprised the linter isn't making you mention animatedValue in the dependencies list.

It's not clear in the docs, but I think this turns out to be expected:

facebook/react#16121 (comment)
facebook/react#14387 (comment)
facebook/react#14387 (comment)

Hmm, those comments explain that we shouldn't put animatedValue.current as a dependency, and I think I understand that.

But what I'd expect the linter to look at in the first place is the local variables that are getting closed over, so animatedValue, not any of its properties. In the first linked comment's terms, that's a ref container, not an example of ref.current.

It's actually just fine to not have it in the dependencies list, because it comes straight from a useRef, and that's guaranteed to give the same value on re-render. So having it as a dependency would never have any effect, because it'll never see a change.

I just wouldn't have expected the linter to detect that nuance, so I'd have expected it to flag this.

... Oh right, but this linter is from the React developers -- it's not something a random person decided to write. So maybe it's not so surprising that they did the work to capture that. And in fact it looks like this (and the other mention of "useRef" a bit lower down) is the pertinent bit of logic. The whole file is 1814 lines long -- so yeah, they weren't slacking.

Interesting; yeah, it's nice to see that they took care writing that lint rule.

gnprice · 2021-07-23T23:35:52Z

src/animation/AnimatedComponent.js

+  useLayoutEffect(() => {
+    if (prevVisible !== visible) {
+      animate();
+    }


Use `useLayoutEffect` [3] instead of `useEffect` because it seems intuitively what we'd want, for starting the animation.

I think this isn't a case where we have a reason to make an exception to the general upstream advice of preferring useEffect.

The thing that useLayoutEffect accomplishes relative to useEffect is that if it updates something, that update takes effect before the next paint.

Here, we're starting an animation. But the thing about an animation is that it begins at zero -- at the instant you start it, it doesn't yet have an effect, and its effects come in gradually. So I think synchronously starting the animation shouldn't have any effect on the current frame. We can let this frame go ahead and paint without trying to hold it up, and then start the animation so it happens over the next few frames.

gnprice · 2021-07-23T23:44:29Z

src/animation/AnimatedComponent.js

+  useLayoutEffect(() => {
+    if (prevVisible !== visible) {
+      animate();


AnimatedComponent: Only call `animate` on visibility change. Instead of on every render, where it often ends up being unnecessary work.

Hmm, I think there is one behavior change here that may not be desirable:

In the old code, if when this component first gets mounted it has visible: true, it will have the value start at 0 and animate up to fullValue.

In the new code, if when this component first gets mounted it has visible: true... hmm, in fact this seems definitely not desirable: I think what will happen is that the value will start at 0 and will stay there, until/unless visible changes to false and then to true again.

One way to look at this is that what we really want here is a usePrevious that doesn't have the funny quirk where on the first call, it takes the "previous" value to be the current one. Instead, it should take another argument to explicitly tell it the "initial" value, and pass that initial value to its useRef.

Then here, we're using the value 0 when we initialize animatedValue; and so for initializing prevVisible we should use the value of visible that corresponds to that, namely false.

In the old code, if when this component first gets mounted it has visible: true, it will have the value start at 0 and animate up to fullValue.

Hmm, interesting. Is this what we want, though? If visible is initially true, might we want to initialize animatedValue.current with new Animated.Value(fullValue) instead of new Animated.Value(0), so it would start as visible, instead of effectively starting as invisible (but with an immediately scheduled animation to become visible)?

Yeah, that does seem better.

Looking at existing uses of this component, there's just one: it's in ComposeMenu, and it always starts with visible false. So we're currently not exercising this case at all. That's good because it means we don't have anything relying somehow on the current kind-of-quirky behavior.

gnprice · 2021-07-23T23:57:43Z

src/common/InputWithClearButton.js

-    this.setState({
-      canBeCleared: text.length > 0,
-      text,
-    });
-    if (this.props.onChangeText) {
-      this.props.onChangeText(text);
-    }
-  };
+  const handleChangeText = useCallback(
+    (_text: string) => {
+      setCanBeCleared(_text.length > 0);
+      setText(_text);


Hmm, so this is a case where we have two state fields that are tied closely together and really should be updated synchronously with each other. Does useState guarantee that if you call two different setState functions one right after the other, nothing else will see a state where just one update has happened? I don't see anything about that in the docs:
https://reactjs.org/docs/hooks-reference.html#usestate

It may be cleanest to make a practice of keeping any such group of state fields together in an object which gets maintained as a single useState state.

In this particular case… it sure looks like canBeCleared is just always the same as text.length > 0. So the best solution here might be a prep commit that just drops canBeCleared from the state, and computes it where we use it.

gnprice · 2021-07-24T00:10:31Z

src/common/SmartUrlInput.js

-  };
+  }, []);
+
+  const renderPlaceholderPart = useCallback(


Maybe skip the useCallback on this one. It's not any kind of event-handler callback -- rather it's a helper for rendering, to render a fragment of the element tree. And both the name, and the contents (consisting mostly of JSX, so producing some React elements to return), make it look very much like that's going to be the case.

gnprice · 2021-07-24T00:23:50Z

src/common/SmartUrlInput.js

+  useFocusEffect(
+    useCallback(() => {
+      if (textInputRef.current) {
        // `.current` is not type-checked; see definition.
-        this.textInputRef.current.focus();
+        textInputRef.current.focus();
      }
-    });
-  }
-
-  componentWillUnmount() {
-    if (this.unsubscribeFocusListener) {
-      this.unsubscribeFocusListener();
-    }
-  }
-
-  handleChange = (value: string) => {
-    this.setState({ value });
+    }, []),
+  );


I didn't find the doc super clear about exactly how useFocusEffect behaves, so I looked at the implementation. It appears to be in node_modules/@react-navigation/core/src/useFocusEffect.tsx.

There's one thing it does which the old code here didn't, but I think has no practical effect for a different reason: if the screen is already focused when the component first mounts, then useFocusEffect will call the callback, while the old code relying on just a listener wouldn't.

(The doc does mention this, lower down.)

Empirically, when I hit the "Add new account" button to open the screen this appears on, the input gets focused anyway (before these changes). I guess that's due to the autoFocus prop we pass to the TextInput.

I think I wouldn't call this commit NFC, because it does take some more investigation (like seeing that autoFocus prop) to see that it doesn't end up changing how the app visibly behaves.

src/common/ZulipButton.js

Use `useRef` for `animatedValue` [1] [2]. Greg points out that we should choose `useEffect` over `useLayoutEffect` [3]. [1] https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables [2] https://reactnative.dev/docs/animated [3] zulip#4914 (comment)

Greg points out [1] that `canBeCleared` is always the same as `text.length > 0`. So, just drop this piece of state and compute the value we need when we need it. [1] zulip#4914 (comment)

Following the doc at https://reactnavigation.org/docs/use-focus-effect/. Greg points out [1] that `useFocusEffect` seems to have the effect of calling the callback even on the component's first mount (if the screen is focused), whereas the old listener-based code would not. That doesn't seem to have a practical effect, here; empirically, it looks like the input gets focused anyway before this change. That's probably due to the `autoFocus` prop we pass to the `TextInput`. [1] zulip#4914 (comment).

chrisbobbe · 2021-07-26T18:56:08Z

Thanks for the review! Revision pushed. Please also see my replies above:

These are equivalent, but Greg prefers this form because it seems cleaner. See https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.

Like we did in 4aedc4c.

Use `useRef` for `animatedValue` [1] [2]. Greg points out that we should choose `useEffect` over `useLayoutEffect` [3]. [1] https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables [2] https://reactnative.dev/docs/animated [3] zulip#4914 (comment)

Greg points out [1] that `canBeCleared` is always the same as `text.length > 0`. So, just drop this piece of state and compute the value we need when we need it. [1] zulip#4914 (comment)

Following the doc at https://reactnavigation.org/docs/use-focus-effect/. Greg points out [1] that `useFocusEffect` seems to have the effect of calling the callback even on the component's first mount (if the screen is focused), whereas the old listener-based code would not. That doesn't seem to have a practical effect, here; empirically, it looks like the input gets focused anyway before this change. That's probably due to the `autoFocus` prop we pass to the `TextInput`. [1] zulip#4914 (comment).

gnprice · 2021-07-28T06:57:37Z

src/RootErrorBoundary.js

-  render(): React$Node {
+  render(): Node {


flow: Use `Node` type exported from React, instead of `React$Node`.

111 files changed, 246 insertions(+), 156 deletions(-)

Eep -- I hope you found a way to automate this!

(I would probably do something like this with a command involving perl -i -0pe.)

I did a VSCode find-and-replace-all :)

(More details in chat: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/codebase-wide.20edits/near/1240343)

gnprice · 2021-07-28T07:02:20Z

src/animation/AnimatedComponent.js

+
+  const prevVisible = usePrevious(visible);
+
+  const animatedValue = useRef(new Animated.Value(0));


Cool, yeah. That looks like something we'd want to do if we actually know the constructor is expensive, but not something we'd want to default to speculatively.

gnprice · 2021-07-28T07:34:24Z

Thanks for the revision! All looks good now modulo the AnimatedComponent issue (#4914 (comment)). I also replied on each of those comment threads you linked to.

Currently, AnimatedComponent has only one caller, and it always starts by passing `false` for `visible`. Since it never passes `true` initially, the new path isn't exercised. But we should make it so future callers that pass `true` initially will actually see the element start as visible. So, do that. See more discussion at zulip#4914 (comment).

Greg points out [1] that `canBeCleared` is always the same as `text.length > 0`. So, just drop this piece of state and compute the value we need when we need it. [1] zulip#4914 (comment)

Following the doc at https://reactnavigation.org/docs/use-focus-effect/. Greg points out [1] that `useFocusEffect` seems to have the effect of calling the callback even on the component's first mount (if the screen is focused), whereas the old listener-based code would not. That doesn't seem to have a practical effect, here; empirically, it looks like the input gets focused anyway before this change. That's probably due to the `autoFocus` prop we pass to the `TextInput`. [1] zulip#4914 (comment).

chrisbobbe · 2021-07-28T15:14:28Z

Thanks for the review! Revision pushed.

It's intentional that both of these expressions do the same thing: the point is that when the component first renders, it should start out in the desired state, and then when the desired state changes and causes a rerender it'll animate to the new desired state. So, we can make that explicit (and avoid some future change causing an accidental discrepancy) by writing the expression just once. Also because `visible` is already boolean, we can skip the `=== true`.

Instead of on every render, where it often ends up being unnecessary work.

…ents.

Using `useRef` for `unsubscribeFocusListener` [1]. We'll simplify the navigation-focus logic with `useFocusEffect` in the next commit. [1] https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables

Following the doc at https://reactnavigation.org/docs/use-focus-effect/. Greg points out [1] that `useFocusEffect` seems to have the effect of calling the callback even on the component's first mount (if the screen is focused), whereas the old listener-based code would not. That doesn't seem to have a practical effect, here; empirically, it looks like the input gets focused anyway before this change. That's probably due to the `autoFocus` prop we pass to the `TextInput`. [1] zulip#4914 (comment).

This is much easier to do now that we've converted these to function components in this series. This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907. Prefer the `Node` type exported from React over `React$Node`. They're equivalent, but Greg prefers `Node` as it seems cleaner; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.

gnprice · 2021-07-28T19:41:18Z

Thanks, looks good! Merging.

I added one small commit further tweaking that AnimatedComponent code:
c7be42f AnimatedComponent [nfc]: Dedupe target-value expression.

chrisbobbe · 2021-07-28T19:42:28Z

Thanks for the review!

I added one small commit further tweaking that AnimatedComponent code:
c7be42f AnimatedComponent [nfc]: Dedupe target-value expression.

Looks great, thanks!

As Greg pointed out a few weeks ago [1], `usePrevious` has had a "funny quirk where on the first call, it takes the "previous" value to be the current one." Instead, let the caller decide what the value should be on the first render, or just have it be `null` if the caller is fine with that. [1] zulip#4914 (comment)

chrisbobbe requested review from gnprice and WesleyAC July 21, 2021 17:57

chrisbobbe mentioned this pull request Jul 21, 2021

Flow: Switch to new "Types-First" mode. #4907

Closed

gnprice reviewed Jul 24, 2021

View reviewed changes

chrisbobbe force-pushed the pr-more-annotations branch from 1cc55eb to f18ca43 Compare July 26, 2021 18:54

chrisbobbe added 9 commits July 27, 2021 10:12

flow: Use Node type exported from React, instead of React$Node.

bf967cb

These are equivalent, but Greg prefers this form because it seems cleaner. See https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.

CreateGroupScreen [nfc]: Inline handleFilterChange.

cf222c4

Like we did in 4aedc4c.

ChooseRecipientsScreen [nfc]: Inline handleFilterChange.

3f3e68b

Like we did in 4aedc4c.

AutocompleteView [nfc]: Convert to function component.

3c95507

ZulipSwitch [nfc]: Convert to function component.

3293476

UsersScreen [nfc]: Convert to function component.

eb88f62

UsersScreen [nfc]: Inline handleFilterChange.

080b29f

Like we did in 4aedc4c.

UserGroupItem [nfc]: Convert to function component.

4bc6dba

chrisbobbe force-pushed the pr-more-annotations branch from f18ca43 to 43a5b55 Compare July 27, 2021 14:28

gnprice reviewed Jul 28, 2021

View reviewed changes

chrisbobbe force-pushed the pr-more-annotations branch from 43a5b55 to b184f62 Compare July 28, 2021 15:14

gnprice and others added 2 commits July 28, 2021 12:38

AnimatedComponent: Only call animate on visibility change.

85d522d

Instead of on every render, where it often ends up being unnecessary work.

chrisbobbe added 22 commits July 28, 2021 12:39

LineSeparator [nfc]: Convert to function component.

726bb5d

LoadingIndicator [nfc]: Convert to a function component.

5db4b92

NestedNavRow [nfc]: Convert to function component.

f51bc79

OptionDivider [nfc]: Pull out componentStyles.

ea65f05

OptionDivider [nfc]: Convert to function component.

45dc89c

PasswordInput [nfc]: Convert to function component.

a421ed6

Popup [nfc]: Convert to a function component.

91145ed

SearchInput [nfc]: Convert to function component.

09e7077

SmartUrlInput [nfc]: Stop choosing new variable names in some assignm…

a7d1087

…ents.

SmartUrlInput [nfc]: Convert to function component.

12bec80

Using `useRef` for `unsubscribeFocusListener` [1]. We'll simplify the navigation-focus logic with `useFocusEffect` in the next commit. [1] https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables

SmartUrlInput [nfc]: Comment on route navigation focus effect.

9effeaf

SwitchRow [nfc]: Pull out componentStyles.

0b01c9e

SwitchRow [nfc]: Convert to function component.

34eb383

UnreadCount [nfc]: Convert to function component.

e2b23ce

UnreadCount [nfc]: Add TODO for tidying up interface.

d3f610f

ZulipButton [nfc]: Convert to function component.

a2f9ca9

EmojiRow [nfc]: Convert to function component.

a66b701

NavButtonGeneral [nfc]: Pull out componentStyles.

a0d5cb7

NavButtonGeneral [nfc]: Convert to function component.

2a55fba

StreamList [nfc]: Convert to function component.

6650e6e

gnprice force-pushed the pr-more-annotations branch from b184f62 to 81a50bb Compare July 28, 2021 19:40

gnprice merged commit 81a50bb into zulip:master Jul 28, 2021

chrisbobbe deleted the pr-more-annotations branch November 4, 2021 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flow: Annotate some more exported React components. #4914

flow: Annotate some more exported React components. #4914

chrisbobbe commented Jul 21, 2021 •

edited

Loading

gnprice left a comment

gnprice Jul 23, 2021

chrisbobbe Jul 26, 2021

chrisbobbe Jul 26, 2021 •

edited

Loading

gnprice Jul 28, 2021

gnprice Jul 23, 2021

chrisbobbe Jul 26, 2021 •

edited

Loading

gnprice Jul 28, 2021

chrisbobbe Jul 28, 2021

gnprice Jul 23, 2021

gnprice Jul 23, 2021

chrisbobbe Jul 26, 2021 •

edited

Loading

gnprice Jul 28, 2021

gnprice Jul 23, 2021

gnprice Jul 24, 2021

gnprice Jul 24, 2021

chrisbobbe commented Jul 26, 2021 •

edited

Loading

gnprice Jul 28, 2021

chrisbobbe Jul 28, 2021

chrisbobbe Jul 29, 2021

gnprice Jul 28, 2021

gnprice commented Jul 28, 2021

chrisbobbe commented Jul 28, 2021

gnprice commented Jul 28, 2021

chrisbobbe commented Jul 28, 2021


		const prevVisible = usePrevious(visible);

		const animatedValue = useRef(new Animated.Value(0));

flow: Annotate some more exported React components. #4914

flow: Annotate some more exported React components. #4914

Conversation

chrisbobbe commented Jul 21, 2021 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Jul 28, 2021

chrisbobbe commented Jul 28, 2021

gnprice commented Jul 28, 2021

chrisbobbe commented Jul 28, 2021

chrisbobbe commented Jul 21, 2021 •

edited

Loading

chrisbobbe Jul 26, 2021 •

edited

Loading

chrisbobbe Jul 26, 2021 •

edited

Loading

chrisbobbe Jul 26, 2021 •

edited

Loading

chrisbobbe commented Jul 26, 2021 •

edited

Loading