flow: Set up to use Flow enums; convert CreateWebPublicStreamPolicy by chrisbobbe · Pull Request #5444 · zulip/zulip-mobile

chrisbobbe · 2022-07-20T00:13:31Z

Following the guide at https://flow.org/en/docs/enums/enabling-enums/ .

There were some surprises with getting ESLint quite on board with Flow enums, and there's at least one place where I haven't yet managed to fix something it does with switch statements, noted in the commit where I convert CreateWebPublicStreamPolicy. But I think I found a good workaround for that; see there.

Apart from making our own code nicer, this should help us be more confident using Flow enums in third-party type definitions. I've been working on some where the library's TypeScript uses TS enums: expo-application and expo-screen-orientation.

chrisbobbe · 2022-07-20T00:29:00Z

The runtime values seem to be working fine; I exercised one of the switch statements on a CreateWebPublicStreamPolicy value in manual testing. Specifically, the one in EditStreamCard; on CZO, I got the message "{realmName} only allows organization owners…", which was expected with CZO's settings.

chrisbobbe · 2022-07-20T03:39:17Z

Fixed CI. In

eslint: Use new hermes-eslint parser, instead of @babel/eslint-parser

, there was some (good) new ESLint feedback that didn't show up in my local testing. My local testing didn't include --all-files. I added details in the commit message.

chrisbobbe · 2022-07-20T04:07:28Z

+        /* $FlowExpectedError[invalid-exhaustive-check]: TODO: Flow points
+           out that the default case can be removed because the other cases
+           are exhaustive. ESLint doesn't agree; fix that and remove the default
+           case. */


flow: Use a Flow enum for the first time! (CreateWebPublicStreamPolicy)

Pasted from the commit message, here's what ESLint reports when I remove the default case:

/Users/chrisbobbe/dev/zulip-mobile/src/streams/EditStreamCard.js 70:15 error Expected to return a value at consistent-return the end of arrow function 71:7 error Expected a default case default-case

Hmm. Thinking more about this, I think it's not realistic to expect consistent-return or default-case (or any ESLint rule) to recognize when the cases are exhaustive. In

eslint: Use new hermes-eslint parser, instead of @babel/eslint-parser

, we helped ESLint lint better by making it model Flow syntax more faithfully. But that doesn't mean ESLint will (or should) take that better model and go and do the type-checking to conclude that the cases are exhaustive. That would mean it's doing the job of a type-checker, and that's not its job.

So: can we repeal or adjust consistent-return and default-case?

Yeah, I think the right answer here is to disable those two rules.

The one thing I see that we'll lose by doing so is that if you have a function whose return type includes void, then I'd like something like consistent-return to still apply: if there are any return statements with arguments, then I'd like to not have other control paths that implicitly return. That just makes it a bit easier when reading the code to be confident of spotting all the ways it returns.

But I don't think that's critical. The type-checker is taking care of everything we need here that's actually critical. We can cheerfully disable these rules.

gnprice · 2022-07-22T23:30:53Z


+  #
+  # ================================================================
+  # Obsolete or redundant rules; repeal or adjust.


nit: This heading reads to me like it's describing a task for us to do in the future, but after reading the contents I believe it's actually describing what this section does now.

I think I'd rather just keep these in the thematic organization we have below: the ft-flow/ lines would go under "Flow; plugin ft-flow.", etc.

I agree about the ft-flow/ lines and the react/ lines, since those are about rules from specific plugins, but I'm not yet sure about no-undef. Copying:

# With hermes-eslint for `parser`, above, this rule can analyze Flow # types. It would fire on global types like $ReadOnly, for which it # doesn't see a declaration. We could find out how to teach the rule that # these types are globally defined…or just turn the rule off, because it's # redundant with Flow (specifically `Flow(cannot-resolve-name)`, I think): # https://github.com/flow-typed/eslint-plugin-ft-flow/issues/33#issue-1274050123 no-undef: off # https://eslint.org/docs/latest/rules/no-undef

no-undef is an out-of-the-box rule from ESLint, it's not from a plugin, so the right section is less obvious to me. It might go under "Airbnb rule repeals / adjustments"? But:

Is Airbnb responsible for bringing in this rule? I see airbnb under extends:, but I also see some other things there that could plausibly be asking for this rule. I guess the most efficient way to find out is to find each line's list of rules it brings in, and search those lists. I didn't find a way to make the ESLint CLI do this for me.

Is it important that Airbnb (etc.; see previous bullet) is bringing in this rule? Or do we want to reject it no matter where it came from, since it's redundant with Flow?

I also see some other things there that could plausibly be asking for this rule

To expand on this, here's the extends::

extends: - airbnb - plugin:jest/recommended - plugin:react-hooks/recommended - ./tools/formatting.eslintrc.yaml

We can easily rule out ./tools/formatting.eslintrc.yaml; it's a path to a short file we maintain. And I already knew it doesn't mention no-undef.

But thinking conservatively about plugin:jest/recommended and plugin:react-hooks/recommended…I'd treat "recommended for use with Jest" and "recommended for use with React Hooks" as not really helping me know what's in the tin. I guess I should read code for eslint-plugin-jest and eslint-plugin-react-hooks in node_modules/, or find those on GitHub and turn to the specific versions we use, with help from yarn why…

…ah, right, so I see that in each case, the plugin's "recommended" list only includes rules defined by the plugin itself, so in particular neither includes no-undef. That makes things neater, but I had to check, because nothing enforces that. (We know nothing enforces it because airbnb is in the same list, and it brings in lots of non-plugin rules that we have existing repeals for.)

…ah, right, so I see that in each case, the plugin's "recommended" list only includes rules defined by the plugin itself, so in particular neither includes no-undef. That makes things neater, but I had to check, because nothing enforces that.

Yeah. Nothing enforces that, but OTOH my sense is that that is a pretty firm convention among ESLint plugins: A ruleset like "plugin:foo/recommended", where "foo" is an ESLint plugin, consists of a subset of the rules that the plugin "foo" itself defines.

Effectively this convention substitutes for having a concept of "this rule is enabled by default" as part of a rule's definition.

Is it important that Airbnb (etc.; see previous bullet) is bringing in this rule?

Yeah, I think the "Airbnb rule repeals / adjustments" section heading may have outgrown its usefulness as part of the organization of this file.

... Hmm, in fact, I think that section never made much sense. It looks like it originates with 245ea9c followed by 1d870dd. Before that former commit, there wasn't anything like an "Airbnb repeals" section -- because that's what most of the rules block was about anyway.
https://github.com/zulip/zulip-mobile/blob/86a42582a/.eslintrc.yaml

So yeah, probably ideally the rules in this "Airbnb repeals" section would all just get moved into the appropriate thematic sections below.

(The "Obsolete or redundant rules" section heading this comment thread was originally about is already gone in the current revision.)

So yeah, probably ideally the rules in this "Airbnb repeals" section would all just get moved into the appropriate thematic sections below.

(That's out of scope for this PR, though.)

That all makes sense; thanks!

gnprice · 2022-07-22T23:47:01Z

+          /* $FlowFixMe[incompatible-use] - CreateWebPublicStreamPolicy is a
+             Flow enum. Values are numbers, so they do have .toString…but
+             Flow reasonably hides that detail from consumers. For
+             CreateWebPublicStreamPolicy, it would actually be better to use
+             CreateWebPublicStreamPolicy.getName(initialStateValue), if we
+             find a nice way to write that with type checking. */
+          const initialStateLabel = initialStateValue?.toString() ?? '[nullish]';
+          // $FlowFixMe[incompatible-use] - As above.
+          const eventLabel = eventValue?.toString() ?? '[nullish]';


I think a good way to handle this would be to pull out a little helper function in this test file to contain this one line's worth of logic, with a name like labelFromValue. If nothing else, that would provide a single place for this comment.

Then that function could potentially grow some conditional logic to decide whether it should call getName instead of toString. Even if there were still some fixmes in the function, that'd be fine -- particularly as this is only in test code, and moreover just in computing the name for the test to use in the test-running UI.

gnprice · 2022-07-23T00:02:14Z

+        /* $FlowExpectedError[invalid-exhaustive-check]: TODO: Flow points
+           out that the default case can be removed because the other cases
+           are exhaustive. ESLint doesn't agree; fix that and remove the default
+           case. */


Yeah, I think the right answer here is to disable those two rules.

The one thing I see that we'll lose by doing so is that if you have a function whose return type includes void, then I'd like something like consistent-return to still apply: if there are any return statements with arguments, then I'd like to not have other control paths that implicitly return. That just makes it a bit easier when reading the code to be confident of spotting all the ways it returns.

But I don't think that's critical. The type-checker is taking care of everything we need here that's actually critical. We can cheerfully disable these rules.

gnprice · 2022-07-23T00:03:13Z

Thanks @chrisbobbe! I am also happy about how much cleaner this definition is, and the switch statements. Comments above.

chrisbobbe · 2022-07-26T01:34:49Z

Thanks for the review! Revision pushed, and I've sent a reply to one of your suggestions, above.

gnprice · 2022-08-03T05:21:01Z

Thanks for the revision!

Sorry for the delay on re-reviewing it, but I've looked just now at the first three commits, and merged those as:
d84ad12 eslint: Use better-maintained fork ("ft-flow") of "flowtyped" plugin
16d1182 flow-todo: Switch parser to @babel/eslint-parser, from babel-eslint
3b76083 eslint: Use new hermes-eslint parser, instead of @babel/eslint-parser

chrisbobbe · 2022-08-17T21:39:55Z

Thanks for the review! I've just rebased atop main now that it includes those three commits; the six commits in this branch are ready for review.

gnprice · 2022-08-31T19:53:00Z

+  #   https://github.com/import-js/eslint-plugin-import/issues/2073
+  # Turn off; seems like this rule's purpose is covered by Flow; discussion:
+  #   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20enums/near/1406529
+  import/named: off #https://github.com/import-js/eslint-plugin-import/blob/v2.26.0/docs/rules/named.md


nit: Is this lack of space in #https://… intentional?

It reads a bit oddly; and it can make the link a bit more annoying to copy-paste into e.g. a browser.

gnprice · 2022-08-31T20:07:05Z


+  #
+  # ================================================================
+  # Obsolete or redundant rules; repeal or adjust.


…ah, right, so I see that in each case, the plugin's "recommended" list only includes rules defined by the plugin itself, so in particular neither includes no-undef. That makes things neater, but I had to check, because nothing enforces that.

Yeah. Nothing enforces that, but OTOH my sense is that that is a pretty firm convention among ESLint plugins: A ruleset like "plugin:foo/recommended", where "foo" is an ESLint plugin, consists of a subset of the rules that the plugin "foo" itself defines.

Effectively this convention substitutes for having a concept of "this rule is enabled by default" as part of a rule's definition.

gnprice · 2022-08-31T20:18:48Z

Thanks for the revision!

All looks good except a small nit mentioned above. Please merge at will.

chrisbobbe · 2022-08-31T21:35:14Z

All looks good except a small nit mentioned above. Please merge at will.

Merged, after fixing that. Thanks for the review!

chrisbobbe added the P1 high-priority label Jul 20, 2022

chrisbobbe requested a review from gnprice July 20, 2022 00:13

chrisbobbe force-pushed the pr-start-flow-enums branch from 38e2f98 to 9df5eb1 Compare July 20, 2022 00:20

chrisbobbe marked this pull request as ready for review July 20, 2022 00:21

chrisbobbe force-pushed the pr-start-flow-enums branch from 9df5eb1 to aa40a62 Compare July 20, 2022 00:28

chrisbobbe force-pushed the pr-start-flow-enums branch 5 times, most recently from 2813c0f to 0c8289b Compare July 20, 2022 03:38

chrisbobbe force-pushed the pr-start-flow-enums branch from 0c8289b to 58abd51 Compare July 20, 2022 03:54

chrisbobbe commented Jul 20, 2022

View reviewed changes

gnprice mentioned this pull request Jul 22, 2022

dep types: Use TsFlower for expo-application and expo-screen-orientation #5447

Merged

gnprice reviewed Jul 23, 2022

View reviewed changes

chrisbobbe force-pushed the pr-start-flow-enums branch from 58abd51 to 0ca2c53 Compare July 26, 2022 01:34

chrisbobbe force-pushed the pr-start-flow-enums branch from 0ca2c53 to d5569ab Compare July 26, 2022 16:30

chrisbobbe force-pushed the pr-start-flow-enums branch from d5569ab to 055d9f3 Compare August 17, 2022 21:38

gnprice reviewed Aug 31, 2022

View reviewed changes

chrisbobbe force-pushed the pr-start-flow-enums branch from 055d9f3 to 9b3267a Compare August 31, 2022 21:09

chrisbobbe closed this Aug 31, 2022

chrisbobbe force-pushed the pr-start-flow-enums branch from 9b3267a to fbea2a2 Compare August 31, 2022 21:35

chrisbobbe deleted the pr-start-flow-enums branch August 31, 2022 21:35

chrisbobbe mentioned this pull request Sep 1, 2022

settings: Add option to mark read on scroll only in conversation views #5469

Merged

Uh oh!

Conversation

chrisbobbe commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jul 20, 2022

Uh oh!

chrisbobbe commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gnprice commented Jul 23, 2022

Uh oh!

chrisbobbe commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Aug 3, 2022

Uh oh!

chrisbobbe commented Aug 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Aug 31, 2022

Uh oh!

chrisbobbe commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe commented Jul 20, 2022 •

edited

Loading

chrisbobbe commented Jul 20, 2022 •

edited

Loading

chrisbobbe Jul 20, 2022 •

edited

Loading

chrisbobbe Jul 26, 2022 •

edited

Loading

chrisbobbe Jul 26, 2022 •

edited

Loading

gnprice Aug 31, 2022 •

edited

Loading

chrisbobbe commented Jul 26, 2022 •

edited

Loading