feat!: only list active members

[RangerMauve]

Closes #431

By default project.$member.getMany() will now only return active members unless you add the includeInactive flag.

I also tried to make the JSDoc types reflect that omitting the flag only yields active members. Hopefully that works and is helpful?

I adjusted some of the tests that were expecting inactive members to be in there. Also added a test to make sure the flag was working as expected outside of the existing tests that depend on it implicitly.

Review requests:

• Does the naming make sense? Feel free to propose other names.
• Are the types looking good?

[gmaclennan]

Using a hard-coded list of roleIds to establish whether a user is part of the project seems fragile. the BLOCKED and LEFT roles are considered "internal" roles, so it might be better to use those roles to establish membership of a project, rather than a the list of other roles, which could change in the future.

I think inactive can be unclear, because a member of a project can be "inactive" but still be a member of the project (this would happen if a device is lost or the app is re-installed, so the existing "member" just doesn't exist any more and will never be seen again). For listProjects() we use the term includeLeft. This could be confusing considering the difference between between project.leave() and project.member.blob(), but maybe consistency is better?

[achou11]

RE: type narrowing based on the specified param...

It'd definitely be nice to have but not a complete blocker. Without it, the client will have to do the filtering or just coerce the type in order to narrow it, which is kind of what's being done anyways. Having the type narrowing implemented would help avoid some hard-to-manage code paths.

Fine with this PR being solely about changing the default behavior of MemberApi.getMany() and having the type narrowing as a nice-to-have follow-up. I went down a rabbithole of getting the narrowing to work without using an overload and got maybe 75% of the way there before moving on 😄 (it's definitely tricky)

[achou11]

In case it's helpful and informs any additional changes to this PR

[achou11]

will the size of allRoles ever get to a point where it's worth doing more optimal iteration instead of the 4(?) array allocations happening here? i guess $member.getMany() may not be a hot path, but what's here stood out to me as something that can be achieved with a single for loop 😄

[achou11]

took a while but think I was able to update the type declaration and implementation such that TypeScript actually checks that the return type is correct based on the input type. In order to get stronger guarantees, it requires separating the creation of return value based on the input parameter:

diff --git a/src/member-api.js b/src/member-api.js
index 77924fd..c2d63bc 100644
--- a/src/member-api.js
+++ b/src/member-api.js
@@ -36,7 +36,7 @@ const ACTIVE_ROLE_IDS = [CREATOR_ROLE_ID, MEMBER_ROLE_ID, COORDINATOR_ROLE_ID]
  *   DeviceInfo,
  *   DeviceInfoValue,
  *   ProjectSettings,
- *   ProjectSettingsValue
+ *   ProjectSettingsValue,
  * } from '@comapeo/schema'
  */
 /** @import { Promisable } from 'type-fest' */
@@ -568,24 +568,19 @@ export class MemberApi extends TypedEmitter {
     return result
   }
 
-  /**
-   * @overload
-   * @param {object} opts
-   * @param {true} opts.includeLeft
-   * @returns {Promise<Array<MemberInfo>>}
-   */
   /**
    * @overload
    * @returns {Promise<Array<ActiveMemberInfo>>}
    */
   /**
+   * @template {boolean} [T=false]
+   *
    * @overload
-   * @param {object} opts
-   * @param {false} opts.includeLeft
-   * @returns {Promise<Array<ActiveMemberInfo>>}
-   */
-  /**
-   * List members in the project. By default only active Members and Coordinators are returned
+   * @param {Object} opts
+   * @param {T} [opts.includeLeft=false]
+   *
+   * @returns {Promise<T extends true ? Array<MemberInfo> : Array<ActiveMemberInfo>>}
+   *
    */
   async getMany({ includeLeft = false } = {}) {
     const [allRoles, allDeviceInfo] = await Promise.all([
@@ -595,12 +590,14 @@ export class MemberApi extends TypedEmitter {
 
     const deviceInfoByConfigCoreId = keyBy(allDeviceInfo, ({ docId }) => docId)
 
-    return Promise.all(
-      [...allRoles.entries()]
-        .filter(
-          ([_, { roleId }]) => includeLeft || ACTIVE_ROLE_IDS.includes(roleId)
-        )
-        .map(async ([deviceId, role]) => {
+    if (includeLeft) {
+      /**
+       * @type {Array<Promise<MemberInfo>>}
+       */
+      const memberInfoPromises = []
+
+      for (const [deviceId, role] of allRoles.entries()) {
+        const getMemberInfo = async () => {
           /** @type {MemberInfo} */
           const memberInfo = { deviceId, role }
 
@@ -624,8 +621,54 @@ export class MemberApi extends TypedEmitter {
           }
 
           return memberInfo
-        })
-    )
+        }
+
+        memberInfoPromises.push(getMemberInfo())
+      }
+
+      return Promise.all(memberInfoPromises)
+    } else {
+      /**
+       * @type {Array<Promise<ActiveMemberInfo>>}
+       */
+      const activeMemberInfoPromises = []
+
+      for (const [deviceId, role] of allRoles.entries()) {
+        if (!isActiveMemberRole(role)) {
+          continue
+        }
+
+        const getMemberInfo = async () => {
+          /** @type {ActiveMemberInfo} */
+          const memberInfo = { deviceId, role }
+
+          try {
+            const configCoreId = await this.#coreOwnership.getCoreId(
+              deviceId,
+              'config'
+            )
+
+            const deviceInfo = deviceInfoByConfigCoreId.get(configCoreId)
+
+            memberInfo.name = deviceInfo?.name
+            memberInfo.deviceType = deviceInfo?.deviceType
+            memberInfo.joinedAt = deviceInfo?.createdAt
+            memberInfo.selfHostedServerDetails =
+              deviceInfo?.selfHostedServerDetails
+          } catch (err) {
+            // Attempting to get someone else may throw because sync hasn't occurred or completed
+            // Only throw if attempting to get themself since the relevant information should be available
+            if (deviceId === this.#ownDeviceId) throw err
+          }
+
+          return memberInfo
+        }
+
+        activeMemberInfoPromises.push(getMemberInfo())
+      }
+
+      return Promise.all(activeMemberInfoPromises)
+    }
   }
 
   /**
@@ -638,6 +681,15 @@ export class MemberApi extends TypedEmitter {
   }
 }
 
+/**
+ * @param {import('./roles.js').Role} role
+ *
+ * @returns {role is ActiveMemberInfo['role']}
+ */
+function isActiveMemberRole(role) {
+  return ACTIVE_ROLE_IDS.includes(role.roleId)
+}
+
 /**
  * @param {string} baseUrl
  * @param {object} options

admittedly one of those unfortunate cases where you're writing code to appease TypeScript a bit. can do a bit more to reuse some logic but otherwise this is the kind of work needed to get TypeScript to be useful 😅 - hopefully helpful to at least see the diff!

Technically, there's still some level of trust in the implementer because we could make the isActiveMemberRole helper lie to us, but unlikely given how small in scope that narrowing function is.

[achou11]

UGH wait nevermind - i miswrote the type declaration as [includeLeft=false] instead of [opts.includeLeft=false], which results in the wrong inferred type when not providing an input opt at all 🙃 Still, the bit about narrowing the return type based on the narrrowing helper can still be useful 😄 EDIT: nevermind that was a separate issue, I miswrote the overload for when the input param is omitted. updated the diff and everything works as initially described now 😄