ActionSheets: Refactor to tighten up types

[WesleyAC]

This will allow us to use the shared messageActionSheet code in the
React Native parts of the codebase, rather than only the webview parts
of the codebase. It's also generally a cleaner and more type-safe
approach.

The test coverage for this seems not great, but I manually tested
clicking on most of the buttons on both styles of action sheet in the
app.

[WesleyAC]

There's still some stuff that's not great about this: particularly, every ButtonDescription requires every possible callback, so we need to, for instance, pass in startEditMessage on every button, even if none of the buttons are going to edit the message. This isn't a new problem though, just something we might want to tighten up in the future. In the meantime, I've sort of hacked my way around it just by passing in (_) => {} when it's unused, which is OK but not great.

Not sure what the best way to do something like this in flow is. Maybe each button should have it's own type, and we should have a buttonData: MessageButtonData | EditButtonData | (etc...) on the ButtonDescription? But that might be pretty verbose, I'm not sure.

[WesleyAC]

The current state of where the ButtonDescription options are used is as follows:

Required by both message and header actionsheets: auth, message, dispatch, _
Only used in message actionsheets: ownUser, startEditMessage
Only used in header actionsheets: subscriptions

However, we probably want to change header actionsheets to stop using message, and use new stream and topic options instead, since it doesn't require anything else, and that's cleaner on the whole.

[WesleyAC]

And just to be abundantly clear on where this stands: I think merging this PR as-is is an improvement (in that it's a step towards #4541), although this refactor is far from complete (I still need to make the changes I mentioned above). I'm going to keep working on this refactor, but I think it makes sense to review and merge this now just so the refactor doesn't turn into an enormous PR.

[gnprice]

Thanks @WesleyAC !

I like splitting showActionSheet into a showHeaderActionSheet and showMessageActionSheet. They naturally have somewhat different types, so splitting them makes it possible to start expressing that clearly. That split, as a commit of its own, would be a good first commit in this refactor.

I'm not such a fan of turning these signatures into having long lists of positional arguments (here 8 of them) -- I think well before that point it typically becomes better to use objects to effectively get named parameters, to avoid having to match up a long somewhat arbitrary sequence between definition and call site. That's part of what backgroundData is doing here. So all these arguments:

      backgroundData.auth,
      backgroundData.mute,
      backgroundData.subscriptions,
      backgroundData.ownUser,
      backgroundData.flags,

would be cleaner as an object, or as part of an object. Perhaps the simplest way to do this would be to pass literally backgroundData itself, but loosen the type required for it from BackgroundData to an inexact object type with just the properties needed, like { auth: Auth, /* … */, flags: FlagsState, ... }. That way the function's signature is expressing that it needs these properties and doesn't care about any others -- and you could add another call site that passes the needed properties without bothering about the other parts of BackgroundData that aren't relevant here.

Zooming out a bit, one thing you'll see we like to do in a refactor like this is to split it into a series of relatively small commits, each self-contained but only expressing a single idea. For some recent examples, take a look at #4437, #4424, or #4442. Here that might look something like:

• Split the function into two; not yet changing the signature at all except there's now two of them, and that first boolean argument is gone.
• Factor out makeButtonCallback.
• Loosen the types of the backgroundData parameters on the various constructFooButtons helpers and also the showFooActionSheet functions.
  • Or, as an example of how I'd make the exact set of changes as this version of the PR but just split them up: one commit that changes the construct* signatures the way this PR does, and then another that changes the show* signatures.
• Drop the narrow argument on showHeaderActionSheet.

I find that strategy helps a lot in reading and understanding and reviewing the changes. It also helps in making it possible to develop larger refactors and manage the complexity of them.

(BTW chat thread here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/shared.20actionsheets.20code/near/1134303 for cross-reference when someone else, or our future selves after a few days from now, is reading this.)

[gnprice]

However, we probably want to change header actionsheets to stop using message, and use new stream and topic options instead, since it doesn't require anything else, and that's cleaner on the whole.

Definitely agreed it'd be good to stop passing it a message, which is unduly specific. It can't be just stream and topic, I think, because you can also get one of these for a PM conversation if you're looking at an interleaved view that contains PMs: all-messages, @-mentions, or search results. Probably the right type for it is Narrow, defined in src/utils/narrow.js.

(And then we should do something in the names to make it clear that this is the narrow the header is about, not the narrow of the surrounding view -- whereas the narrow: Narrow that the message case uses is the latter.)

[WesleyAC]

@gnprice ok, I've split it up into multiple commits and changed most of the functions to take objects as args instead of positional args, should be ready for a review.

I'm not sure what the preferred way of writing a function that takes only an object argument is — I've opted to use destructuring, and duplicate the names of the args in the function signature, but passing a named args parameter would also work. Is there a way to do the destructuring without the verbosity of repeating the argument names?

[WesleyAC]

Perhaps the simplest way to do this would be to pass literally backgroundData itself, but loosen the type required for it from BackgroundData to an inexact object type with just the properties needed, like { auth: Auth, /* … */, flags: FlagsState, ... }. That way the function's signature is expressing that it needs these properties and doesn't care about any others -- and you could add another call site that passes the needed properties without bothering about the other parts of BackgroundData that aren't relevant here.

Oh, hmm, I didn't do it this way, but that does seem cleaner — I'll add another commit to change it to work like this.

[WesleyAC]

Ok, now it should be all good :)

[WesleyAC]

Just manually tested the new commits, everything appears to work as expected.

[WesleyAC]

Since this is split into separate commits, I'm just keeping on pushing refactoring commits here, let me know if that's not the correct thing to do. I figure we can merge them a few at a time, if we need/want to.

[WesleyAC]

Ok, I think I'm done for now — this is a pretty large stack of commits, but each individual one should be pretty easy to review, I think. Let me know if I should bias more towards waiting for feedback before making a big commit stack in the future.

[gnprice]

Thanks for the revision! I appreciate how nicely split out these changes are.

I've read through to about here:
c261b26 ActionSheets: Split showActionSheet into multiple functions
489b6d2 ActionSheets: Factor out makeButtonCallback function
a6956b0 ActionSheets: Refactor construct*ActionButtons to avoid BackgroundData
0096646 ActionSheets: Replace ConstructSheetParams with more specific types
664626c ActionSheets: Remove ButtonCode, pass around objects instead
99efbd4 ActionSheets: Change ButtonDescription type to be more specific
963c978 ActionSheets: Have constructHeaderActionButtons stop taking message arg

and am about to go make and eat dinner, and I figure you may be online before I'm back at my desk. So I wanted to send you the comments I have so far. The remaining commits I haven't looked closely at are:
3f443c6 ActionSheets: Get rid of makeButtonCallback function
63d03d5 ActionSheets: Change HeaderButton callback to take stream and topic
769d85b ActionSheets: Remove message arg from showHeaderActionSheet
a7d152e ActionSheets: Remove startEditMessage from showHeaderActionSheet

Let me know if I should bias more towards waiting for feedback before making a big commit stack in the future.

This is about right for the scope to send as one PR (when the overall scope of a project is this or more, that is -- smaller PRs are of course great when the work can be done in something smaller 🙂 ).

I think the key thing to aim for is to try to post as a PR, or else post something on Zulip about what you've done / what's hard / what you've learned / etc., at least every day that you're working on a given thing. So if it's getting toward the end of your day and there's something you spent time on that day and haven't posted, then try to take some initial segment of those commits and polish them up to send as a PR.

[gnprice]

Seeing this duplication, I realize that it would have been a bit cleaner to have factored this piece out first and then split this function into two versions, rather than vice versa as I suggested above. 🙂 This is fine, though -- it's not a huge amount of duplication, and it goes away in the next commit.

[WesleyAC]

I ended up actually keeping the duplication, since that turned out much easier than making the types work out once we had more than one type of button — that's worth the pretty small amount of duplication, IMO (although if there's a nice flow way to write it, I'm happy to see it)

[gnprice]

Sent #4566 with a way to do this. I agree the duplication isn't a big problem because the code is so small. But the Alert.alert bit in particular feels like the sort of thing that can easily get annoyingly out of sync when duplicated, so I think there's some value in keeping it deduplicated.

[WesleyAC]

I've tested that the final commit here works on my android device, and git rebase origin/master --exec 'tools/test' reports that tests pass at every commit. Should be ready for another review.

[gnprice]

Thanks for the revisions! This all looks great, modulo the nit below. I'm going to go ahead and merge with just a small commit on top fixing that.

I also see your question at #4541 (comment) above, and after merging I'll take a few minutes to see what I can come up with.

[gnprice]

nit: make explicitly inexact (here and a few others below)