Page MenuHomePhabricator

[lib] Update `getRelativeMemberInfos` to handle both `MinimallyEncoded` and `Legacy` variants
ClosedPublic

Authored by atul on Dec 7 2023, 11:53 AM.
Tags
None
Referenced Files
F3157213: D10235.diff
Tue, Nov 5, 7:38 PM
Unknown Object (File)
Mon, Oct 28, 12:50 AM
Unknown Object (File)
Tue, Oct 15, 10:07 PM
Unknown Object (File)
Tue, Oct 15, 10:07 PM
Unknown Object (File)
Tue, Oct 15, 10:07 PM
Unknown Object (File)
Tue, Oct 15, 10:07 PM
Unknown Object (File)
Tue, Oct 15, 10:07 PM
Unknown Object (File)
Tue, Oct 15, 10:07 PM
Subscribers
None

Details

Summary

This is just a copy/paste step so making it a separate diff.

I tried a lot of different ways to "parameterize" existing getRelativeMemberInfos based on Legacy or MinimallyEncoded RawThreadInfo that was being passed in.

Some approaches I looked at:
- $PropertyType<T, 'k'> which I'd previously seen used in dispatchActionPromise. Unfortunately, that didn't work because T.members would give us $ReadOnlyArray<*MemberInfo> instead of $ReadOnlyArray<*RelativeMemberInfo> because the list of *RelativeMemberInfos is in ThreadInfo not RawThreadInfo. I basically couldn't find a way to "map" RawT to T.members (eg RawThreadInfo -> ThreadInfo["members"]). The newer indexed access syntax (https://flow.org/en/docs/types/indexed-access/) also wasn't useful.
- Tried using minimallyEncoded checks within, but flow wasn't able to narrow from *ThreadInfo.minimallyEncoded to *MemberInfo.minimallyEncoded to a *RelativeMemberInfo.minimallyEncoded return type.
- Tried annotating types within function body to see if that would "help" flow in some way?

It looks like what we'd really want is to use Conditional Types which were introduced in Flow 208: https://flow.org/en/docs/types/conditional/

For now I'm going to start by completely duplicating the function, which I know works... and then recursively try to break it down into reusable pieces while preserving the type narrowing.

Updating this diff with the version of getRelativeMemberInfos that @ashoat wrote in the comments here: https://phab.comm.dev/D10235#297963


Depends on D10233

Test Plan

Noop, flow, CI

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D10235 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Dec 7 2023, 1:03 PM
ashoat requested changes to this revision.Dec 8 2023, 2:57 PM

I'd like to try my hand at parameterizing getRelativeMemberInfos. What's the best way to do it? Won't I need at least one usage of the minimally-encoded variant to be able to test that the types work out? Should I check out a later diff?

Passing back to you with question

lib/selectors/user-selectors.js
78–90 ↗(On Diff #34391)

Same comment as here

This revision now requires changes to proceed.Dec 8 2023, 2:57 PM

I'd like to try my hand at parameterizing getRelativeMemberInfos. What's the best way to do it? Won't I need at least one usage of the minimally-encoded variant to be able to test that the types work out? Should I check out a later diff?

You could patch in D10285 which is later in the stack and has no flow issues. In D10272 I introduced a new getRelativeMemberInfos that basically calls getMinimallyEncodedRelativeMemberInfos or getLegacyRelativeMemberInfos based on threadInfo that's passed in. This getRelativeMemberInfos that handles both LegacyRawThreadInfo and MinimallyEncodedRawThreadInfo is consumed in baseRelativeMemberInfoSelectorForMembersOfThread.

Both getLegacyRelativeMemberInfos and getMinimallyEncodedRelativeMemberInfos are effectively identical. You could try

  1. Pasting the contents of either getLegacyRelativeMemberInfos or getMinimallyEncodedRelativeMemberInfos into getRelativeMemberInfos
  2. Try to parametrize getRelativeMemberInfos such that flow issue in baseRelativeMemberInfoSelectorForMembersOfThread is resolved.

Let me know if there's anything else I can clarify or if I'm misunderstanding anything

Realizing my explanation above is probably difficult to follow.

I just created a diff (https://phab.comm.dev/D10291) that you can patch in and should be a more useful place to start from.

I basically replaced the contents of getRelativeMemberInfos with the contents of get[Legacy/MinimallyEncoded]RelativeMemberInfos (which are effectively identical) and replace usages of those with the newly updated getRelativeMemberInfos.

That should narrow things down to just the 12 relevant flow issues.

Got it working with this patch: https://gist.github.com/Ashoat/38315c14a5a16974b840aff796a814eb

type ExtractArrayParam = <T>(arr: $ReadOnlyArray<T>) => T;

function getRelativeMemberInfos<
  TI: MinimallyEncodedRawThreadInfo | LegacyRawThreadInfo,
>(
  threadInfo: ?TI,
  currentUserID: ?string,
  userInfos: UserInfos,
): $ReadOnlyArray<$ReadOnly<{
  ...$Call<
    ExtractArrayParam,
    $PropertyType<TI, 'members'>,
  >,
  +username: ?string,
  +isViewer: boolean,
}>> {
  const relativeMemberInfos: Array<$ReadOnly<{
    ...$Call<
      ExtractArrayParam,
      $PropertyType<TI, 'members'>,
    >,
    +username: ?string,
    +isViewer: boolean,
  }>> = [];
  if (!threadInfo) {
    return relativeMemberInfos;
  }
  const memberInfos = threadInfo.members;
  for (const memberInfoInput of memberInfos) {
    const memberInfo: $Call<
      ExtractArrayParam,
      $PropertyType<TI, 'members'>,
    > = memberInfoInput;
    if (!memberInfo.role) {
      continue;
    }
    const username: ?string = userInfos[memberInfo.id]
      ? userInfos[memberInfo.id].username
      : null;
    const isViewer: boolean = memberInfo.id === currentUserID;
    const relativeMemberInfo: $ReadOnly<{
      ...$Call<
        ExtractArrayParam,
        $PropertyType<TI, 'members'>,
      >,
      +username: ?string,
      +isViewer: boolean,
    }> = {
      ...memberInfo,
      username,
      isViewer,
    };
    relativeMemberInfos.unshift(relativeMemberInfo);
  }
  return relativeMemberInfos;
}

(Did that before reading your most recent comment – not sure if it matters, let me know if it does)

Use @ashoat's implementation of getRelativeMemberInfos instead of splitting into two

atul retitled this revision from [lib] Introduce separate `Legacy` and `MinimallyEncoded` variants of `getRelativeMemberInfos` to [lib] Update `getRelativeMemberInfos` to handle both `MinimallyEncoded` and `Legacy` variants.Dec 13 2023, 2:18 PM
atul edited the summary of this revision. (Show Details)
atul added inline comments.
lib/selectors/user-selectors.js
74–86 ↗(On Diff #34596)

Actually, @ashoat do we need to maintain the order created here with use of unshift and push?

preemptively fix flow issue that pops up later in stack because of changes to this diff

Please address comments before landing

lib/selectors/user-selectors.js
68 ↗(On Diff #34599)

In retrospect we may be able to remove this line (and line 75 and line 97)

74–86 ↗(On Diff #34596)

Oops, yes – good catch, you're definitely right. Can you revert back to the previous order? Hopefully shouldn't result in any new type errors

This revision is now accepted and ready to land.Dec 14 2023, 9:42 AM

maintain previous order w/ unshift/push

lib/selectors/user-selectors.js
68 ↗(On Diff #34599)

When I tried doing that, I got the following flow errors: https://gist.github.com/atulsmadhugiri/6ba7892f2a1bb0d5d7c215d7ffc212cb#file-getrelativememberinfos-flow-errors

Going to land as is, but open to making any changes in followup diff

This revision was landed with ongoing or failed builds.Dec 14 2023, 10:20 AM
This revision was automatically updated to reflect the committed changes.