Annotate more exports. by chrisbobbe · Pull Request #4948 · zulip/zulip-mobile

chrisbobbe · 2021-08-05T21:42:59Z

I promise we're getting nearer to done with these! 🙂

After this, and after the currently open PRs #4943, #4942, #4926, and after we do something about ZulipAsyncStorage (see discussion and #4947), there will be just a few small things that I expect will basically disappear naturally with #4898 and #4868 (and maybe a third PR that's not occurring to me right now—if so, it's tiny).

This also adds a lot of read-only annotations that aren't strictly necessary for our work on annotating things for #4907. But I figured it's a good thing to do, and I'd happily split that off to a different PR if you'd like.

Related: #4907

gnprice

Thanks for going through these! Small comments below. Probably the main thing is that I'd like to avoid duplicating a lot of the type annotations, where we already have annotations that are just not quite in the spot that types-first needs them to be in.

gnprice · 2021-08-11T23:41:54Z

    });

-  getFilteredLanguageList = (filter: string): Language[] => {
+  getFilteredLanguageList: string => Language[] = (filter: string): Language[] => {


nit: can drop the annotations inside the function expression, so that we're effectively moving them instead of duplicating them.

(Similarly just above, and in some other spots in this commit.)

I'm actually kind of surprised that types-first isn't able to use the annotations that are there on the function expression. But 🤷

Still have a redundancy at this one 🙂 :

- getFilteredLanguageList = (filter: string): Language[] => { + getFilteredLanguageList: string => Language[] = (filter: string) => {

Also isSendButtonEnabled.

Indeed! Thanks.

gnprice · 2021-08-11T23:47:51Z

+export const getHtmlPieceDescriptorsForMessages: (
+  $ReadOnlyArray<Message | Outbox>,
+  Narrow,
+) => $ReadOnlyArray<HtmlPieceDescriptor> = defaultMemoize(
  (messages: $ReadOnlyArray<Message | Outbox>, narrow: Narrow) =>


Similarly, can drop the inner copy of these annotations now that there's an outer copy.

... In fact, after doing that, the inner function expression isn't doing anything useful -- we can just pass to defaultMemoize the function getHtmlPieceDescriptors itself.

Which also suggests that instead of …ForMessages the name should be like …Memoized. And perhaps means we should be using the memoized version everywhere, too.

gnprice · 2021-08-12T00:06:34Z

 /* eslint-disable semi-style */
-export type NarrowElement =
+export type NarrowElement = $ReadOnly<
 | {| +operator: 'is' | 'in' | 'topic' | 'search', +operand: string |}


This $ReadOnly is redundant with the marks on each of the individual properties, where it's spelled +. Here's where that syntax is documented:
https://flow.org/en/docs/types/interfaces/#toc-interface-property-variance-read-only-and-write-only

The per-property version is actually the fundamental one -- Flow doesn't have a notion of an object type as a whole being read-only, only of individual properties being that way. When we say a whole type is read-only, it's a shorthand for saying all its properties are. And similarly, $ReadOnly's meaning is defined in terms of making each of the properties read-only:
https://flow.org/en/docs/types/utilities/#toc-readonly

(Similarly at a couple of widget-related types below.)

Hmm, thanks! That makes sense.

As it says in the commit message, I intended to make changes only where Flow specifically suggested them. But I don't see an error if I revert this change, and the others I think you've indicated. 🤷

This is in modelTypes, which is one of the files that it looks like you started with (which makes sense, because these are primarily the types we use to cast the arbitrary JSON we get from the server.)

Oh, got it, right!

gnprice · 2021-08-12T00:08:57Z

+export type TypingState = $ReadOnly<{|
  [normalizedRecipients: string]: {|
    time: number,
    userIds: UserId[],
  |},
-|};
+|}>;


The inner objects here should also get read-only marks. Easy to miss those 😉

Ah, right. I've found a lot more of these, I think all of them. Thanks!

chrisbobbe · 2021-08-12T21:04:42Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Small comment above:
#4948 (comment)
and some others below.

Some of these represent yet more opportunities to add read-only marks. There's no need for those to all happen atomically (beyond what's needed to keep Flow passing), so it's fine to merge some changes now and handle more stragglers later.

gnprice · 2021-08-12T21:25:11Z

-        st = insert(st, keyOfExactUsers(r.user_ids), r.max_message_id);
+        st = insert(
+          st,
+          keyOfExactUsers(
+            r.user_ids
+              // Copy the array (keyOfExactUsers sorts its input in-place)
+              .map(id => id),
+          ),
+          r.max_message_id,
+        );


Hmm, awkward that this makes this one line sprawl across 9 lines. I'd take that as a cue to pull out some subexpression as a local 😉

E.g. like this:

for (const r of action.data.recent_private_conversations ?? []) { - st = insert( - st, - keyOfExactUsers( - r.user_ids - // Copy the array (keyOfExactUsers sorts its input in-place) - .map(id => id), - ), - r.max_message_id, - ); + const key = keyOfExactUsers([...r.user_ids]); + st = insert(st, key, r.max_message_id); }

I think the comment isn't actually needed, because after all the types say we need a mutable array and that we have a read-only one. This bit of code doesn't actually need to care about why the helper demands a mutable array.

(Meanwhile although the map works fine, the spread feels more direct to me, and also seems more likely to be reliably efficient -- no callback to invoke for each element.)

gnprice · 2021-08-12T21:29:04Z

  avatar_url_medium: string,
  can_create_streams: boolean,
-  cross_realm_bots: Array<{| ...CrossRealmBot, avatar_url?: string | null |}>,
+  cross_realm_bots: $ReadOnlyArray<{| ...CrossRealmBot, avatar_url?: string | null |}>,


Yet another layer of nesting: this avatar_url property isn't read-only 😛

This is a good application for the + syntax -- where there's just one or two properties and the whole object type fits on a line, using + vs $ReadOnly can often save it from spilling into several lines.

gnprice · 2021-08-12T21:31:02Z

 | {| +operator: 'group-pm-with', +operand: string | UserId |}
- | {| +operator: 'near' | 'id', +operand: number |} // message ID
- ;
+ | {| +operator: 'near' | 'id', +operand: number |}; // message ID


nit: small noise formatting change

(The existing formatting here is intentional, for making things slightly easier when adding more branches.)

gnprice · 2021-08-12T21:32:52Z

+export type TypingState = $ReadOnly<{|
+  [normalizedRecipients: string]: $ReadOnly<{|
    time: number,
    userIds: UserId[],


Another bit of still-deeper nesting: that UserId array.

chrisbobbe · 2021-08-12T22:35:17Z

Thanks for the review! Revision pushed.

This way, we won't have to annotate `defaultProps` for zulip#4907 in either of the following ways: - by using `$Shape`, which is unsound, - by spelling out the properties that happen to appear there, - or by doing something more complicated, like with `$ObjMap` This has been the reason for converting several other components on the way to zulip#4907, but we haven't written it down until now.

This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907.

…them.

This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907. And, since one consumer doesn't like the annotation, just repeat NULL_OBJECT's expression there, this time with a note about why we're using `Object.freeze`.

This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907. And chase through many Flow errors pointing out that various other object and array types should be marked read-only too. All of them look like they should be read-only anyway; our read-only markers haven't generally kept up with all the places we pass around objects and arrays expecting them not to be mutated. After adding the annotation, I went through five iterations of - run `yarn flow` - resolve each error in the output (and no others) by command-clicking to the type that Flow says should be made read-only, and making it so , stopping when Flow didn't show any more errors.

We like to avoid mutating our Redux actions. As Greg says [1], > The downside there is that it [means] allocating new copies of > each of these arrays. If this were something potentially giant -- > like if this were in the `unread_msgs` data -- then I'd want to > find ways of avoiding that, even at the cost of small hacks, > because it could save time at startup on the scale of like > 50-100ms. > > [...] > > But this isn't something that's going to have the kind of > performance impact to justify that. You'd have to have thousands > of distinct recent PM conversations, and/or group-PM threads with > thousands of users. This will let us make `RecentPrivateConversations.user_ids` a `$ReadOnlyArray`, which we'll do in the next commit. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.2Efilter.20on.20a.20.24ReadOnlyArray.3F/near/1246497

Just because it seems like we should. :) I started by adding read-only marks to every likely-looking type in the following files: - initialDataTypes.js - modelTypes.js - reduxTypes.js Then, similar to the previous commit, I ran Flow and resolved errors surgically by making types read-only where Flow complained that they should be.

Just because it seems like we should. I think this might be all of them! :-) And Flow doesn't point out any followup.

And add a comment saying that it should be preferred. See zulip@5471ec8#r687267624.

gnprice · 2021-08-12T23:34:55Z

Thanks! Looks good -- merging.

There was still a small noise diff at NarrowElement:

@@ -351,7 +351,7 @@ export type NarrowElement =
  | {| +operator: 'sender', +operand: string | UserId |}
  | {| +operator: 'group-pm-with', +operand: string | UserId |}
  | {| +operator: 'near' | 'id', +operand: number |} // message ID
- ;
+;
 
 /**

but I just fixed that before merge.

chrisbobbe · 2021-08-13T15:31:28Z

Oh, oops—thanks for the fix and review! 🙂

chrisbobbe requested review from WesleyAC and gnprice August 5, 2021 21:43

gnprice reviewed Aug 12, 2021

View reviewed changes

chrisbobbe force-pushed the pr-more-annotations-asdf branch from 61af7e9 to 725ec75 Compare August 12, 2021 21:04

gnprice reviewed Aug 12, 2021

View reviewed changes

chrisbobbe force-pushed the pr-more-annotations-asdf branch from 725ec75 to c6ee4cb Compare August 12, 2021 22:35

chrisbobbe added 12 commits August 12, 2021 16:31

NavButton [nfc]: Bring out componentStyles.

fa3c3fc

flow: Annotate some more exported React components.

e7b0093

This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907.

flow: Annotate some more exports.

f64e10c

This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907.

NavigationService types [nfc]: Annotate some exports.

66aad74

This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907.

directSelectors types [nfc]: Use type aliases instead of duplicating …

f3d4f4b

…them.

types: Mark more of our Redux sub-state types read-only.

b6552e9

Just because it seems like we should. I think this might be all of them! :-) And Flow doesn't point out any followup.

msglist: Rename getHtmlPieceDescriptors{ForMessages,Memoized}.

c747185

And add a comment saying that it should be preferred. See zulip@5471ec8#r687267624.

gnprice force-pushed the pr-more-annotations-asdf branch from c6ee4cb to c747185 Compare August 12, 2021 23:34

gnprice merged commit c747185 into zulip:master Aug 12, 2021

gnprice mentioned this pull request Aug 13, 2021

api types: Mark most remaining types read-only #4957

Merged

chrisbobbe mentioned this pull request Aug 13, 2021

Finish removing connect calls for exported components, where it's easy. #4926

Merged

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

Uh oh!

Conversation

chrisbobbe commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 12, 2021

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 12, 2021

Uh oh!

gnprice commented Aug 12, 2021

Uh oh!

chrisbobbe commented Aug 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe commented Aug 5, 2021 •

edited

Loading