Page MenuHomePhabricator

[keyserver, lib] extract checking if message is an edit, reaction or sidebar_source
ClosedPublic

Authored by inka on Apr 26 2023, 7:06 AM.
Tags
None
Referenced Files
F1865530: D7632.id.diff
Sun, May 26, 10:07 AM
Unknown Object (File)
Fri, May 24, 11:03 AM
Unknown Object (File)
Sun, May 19, 5:43 PM
Unknown Object (File)
Sun, May 19, 5:43 PM
Unknown Object (File)
Sun, May 19, 5:43 PM
Unknown Object (File)
Sat, May 18, 2:02 AM
Unknown Object (File)
Fri, May 17, 11:39 PM
Unknown Object (File)
Fri, May 17, 11:39 PM
Subscribers

Details

Summary

In response to review on D7584 I am fixing a invariant message and extracting checking if message is an edit, reaction or sidebar_source.

I cannot use the assertion pattern (using invariants like in assertComposableRawMessage) because in D7584 I need to check the type, not assert it.
I cannot have a function that checks the type and returns a boolean, because then in message-fetchers in line 765 flow is not convinced that the type is correct, because the check is
done elsewhere, and it just sees that the type is RawMessageInfo, which is too broad. Same for sidebar-source-message

I don't think this is a great solution, but I didn't find a better one.

Test Plan

run flow check in keyserver/ and lib/

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This comment was removed by inka.
inka requested review of this revision.Apr 26 2023, 7:25 AM
inka planned changes to this revision.Apr 26 2023, 7:26 AM
bartek requested changes to this revision.Apr 26 2023, 7:34 AM

Couldn't this function be simply:

function isMessageSidebarSourceReactionOrEdit(
  message: MessageInfo | RawMessageInfo
): boolean {
  return (
    message.type === messageTypes.SIDEBAR_SOURCE ||
    message.type === messageTypes.REACTION ||
    message.type === messageTypes.EDIT_MESSAGE
  );
}

// then usages
if (isMessageSidebarSourceReactionOrEdit(messageInfo)) { ... }

invariant(
  !isMessageSidebarSourceReactionOrEdit(message),
  "error text here",
);

?

I did something similar in D7616

We don't need this check for our logic, we only need it to reassure flow that type is correct. We know it is correct. And with the solution you propose flow doesn't know the type is correct inside of the if in message-fetchers.js

In D7632#226400, @inka wrote:

We don't need this check for our logic, we only need it to reassure flow that type is correct. We know it is correct. And with the solution you propose flow doesn't know the type is correct inside of the if in message-fetchers.js

Ah, so maybe try marking this as "predicate function":

function isMessageSidebarSourceReactionOrEdit(
  message: MessageInfo | RawMessageInfo
): boolean %checks {
  return (
    message.type === messageTypes.SIDEBAR_SOURCE ||
    message.type === messageTypes.REACTION ||
    message.type === messageTypes.EDIT_MESSAGE
  );
}

Although, as we talked, flows documentation states that this shouldn't work (here), I tried it and it works.
I ran flow check in lib/ and keyserver/ to be sure there are no errors.

In D7632#226887, @inka wrote:

Although, as we talked, flows documentation states that this shouldn't work (here), I tried it and it works.
I ran flow check in lib/ and keyserver/ to be sure there are no errors.

Awesome!!

I thought it wouldn't work because Flow docs mention that it isn't able to deduce an object's properties' types, but it looks like it is able to do so for the object itself, based on its properties.

This revision is now accepted and ready to land.Apr 27 2023, 7:19 AM