refactor(connected-users): rework user fetching

- Fully tested and encapsulating the cache state in a class
- Cleaner and more elegant implementation to handle cached users vs
  fetched users.

Author: mjpieters

scripts/connected-users/src/users/api.ts

@@ -1,58 +1,87 @@
 /*
  * Fetch users from the Stack Exchange API
  */
-import { LruCache } from '../utils'
+import { Cache, LruCache } from '../utils'
 import { StackExchangeAPI } from '../seAPI'
 import { minimalUserFilter, seAPIKey, userAPICacheSize } from '../constants'
 
-import { DeletedUser, ExistingUser, User } from './classes'
+import { DeletedUser, ExistingUser, JSONLoadable, User } from './classes'
 
-type JSONUser = Parameters<(typeof ExistingUser)['fromJSON']>[0]
-const apiCache = new LruCache<number, User>(userAPICacheSize)
-const api = new StackExchangeAPI(seAPIKey)
+type UserFetcherOptions = {
+  api: StackExchangeAPI
+  cache: Cache<User['user_id'], User>
+  missingAssumeDeleted: boolean
+}
 
-// Fetch users and yield User objects in the same order they are listed in the argument
-export async function* fetchUsers(
-  userIds: number[],
-  missingAssumeDeleted = false
-): AsyncGenerator<User, void, undefined> {
-  // split into still-cached and to-be-fetched uids
-  let toFetch: number[] = []
-  const cached = new Map(
-    userIds.reduce((resolved, uid) => {
-      const user = apiCache.get(uid)
-      if (user) return [...resolved, [uid, user]]
-      toFetch.push(uid)
-      return resolved
-    }, [] as [number, User][])
-  )
-  // fetch the remaining uids in batches of 100
-  while (toFetch.length > 0) {
-    const queryIds = toFetch.splice(0, 100)
-    toFetch = toFetch.splice(100)
-    const results = await api.fetch<JSONUser>(
-      `/users/{ids}`,
-      { ids: queryIds },
-      { filter: minimalUserFilter }
-    )
+export class UserFetcher<
+  UserType extends JSONLoadable<User> = typeof ExistingUser,
+  MissingUser extends User = DeletedUser
+> {
+  private readonly api: StackExchangeAPI
+  private readonly cache: Cache<User['user_id'], User>
+  readonly missingAssumeDeleted: boolean = false
+
+  constructor(
+    private readonly UserClass: UserType,
+    private readonly MissingClass: new (userId: User['user_id']) => MissingUser,
+    options: UserFetcherOptions
+  ) {
+    this.api = options.api
+    this.cache = options.cache
+    this.missingAssumeDeleted = options.missingAssumeDeleted
+  }
+
+  static withDefaultClasses(
+    options: Partial<UserFetcherOptions> = {}
+  ): UserFetcher {
+    const config: UserFetcherOptions = {
+      api: options.api || new StackExchangeAPI(seAPIKey),
+      cache:
+        options.cache || new LruCache<User['user_id'], User>(userAPICacheSize),
+      missingAssumeDeleted:
+        options.missingAssumeDeleted === undefined
+          ? false
+          : options.missingAssumeDeleted,
+    }
+    return new UserFetcher(ExistingUser, DeletedUser, config)
+  }
+
+  /** Fetch users and yield User objects in the same order they are listed in the argument */
+  async *users(
+    userIds: InstanceType<UserType>['user_id'][]
+  ): AsyncIterableIterator<User> {
+    const toFetch: number[] = []
     const byUserId = new Map(
-      results.map((user) => [user.user_id, ExistingUser.fromJSON(user)])
+      userIds.reduce((resolved, uid) => {
+        const user = this.cache.get(uid)
+        if (user === undefined) toFetch.push(uid)
+        return user !== undefined ? [...resolved, [uid, user]] : resolved
+      }, [] as [User['user_id'], User][])
     )
-    const lastFetched = userIds.indexOf(queryIds[queryIds.length - 1]) + 1
-    yield* userIds.splice(0, lastFetched).reduce((mapped, uid) => {
-      let user = cached.get(uid) || byUserId.get(uid)
-      if (user === undefined && missingAssumeDeleted) {
-        user = new DeletedUser(uid)
+    if (toFetch.length > 0) {
+      const apiResults = this.api.fetchAll<Parameters<UserType['fromJSON']>[0]>(
+        '/users/{ids}',
+        { ids: toFetch },
+        { filter: minimalUserFilter }
+      )
+      for await (const jsonUser of apiResults) {
+        const user = this.UserClass.fromJSON(jsonUser)
+        this.cache.put(user.user_id, user)
+        byUserId.set(user.user_id, user)
       }
-      if (user) apiCache.put(uid, user)
-      return user ? [...mapped, user] : mapped
-    }, [])
-    userIds = userIds.splice(lastFetched)
+    }
+    const get = this.missingAssumeDeleted
+      ? (uid: number) => byUserId.get(uid) || this.missingUser(uid)
+      : (uid: number) => byUserId.get(uid)
+    for (const uid of userIds) {
+      const user = get(uid)
+      if (user) yield user
+    }
+  }
+
+  private missingUser(userId: User['user_id']): MissingUser {
+    const missing = new this.MissingClass(userId)
+    this.cache.put(missing.user_id, missing)
+    return missing
   }
-  // yield any remaining cached users
-  yield* userIds.map(
-    (uid) =>
-      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-      apiCache.get(uid)!
-  )
 }

scripts/connected-users/src/users/controller.ts

@@ -1,11 +1,12 @@
 /* global Stacks */
 import { controllerId } from '../constants'
 
-import { fetchUsers } from './api'
+import { UserFetcher } from './api'
 
 // set padding inside the user cards to 0 in the usercard lists
 // _uc-p is the CSS variable set by Stacks and we can override it here.
 const userStyles = `.s-${controllerId} .s-user-card { --_uc-p: 0 }`
+const api = UserFetcher.withDefaultClasses({ missingAssumeDeleted: true })
 
 export class UserListController extends Stacks.StacksController {
   static controllerId = `${controllerId}-user-list`
@@ -44,7 +45,7 @@ export class UserListController extends Stacks.StacksController {
     )
     if (hydrationRows.size === 0) return
     window.requestAnimationFrame(async () => {
-      for await (const user of fetchUsers([...hydrationRows.keys()], true)) {
+      for await (const user of api.users([...hydrationRows.keys()])) {
         const userRow = hydrationRows.get(user.user_id)
         if (!userRow) continue
         const firstChild =

scripts/connected-users/src/utils.ts

@@ -44,7 +44,11 @@ export const outlets = <T>({ controllerId }: HasControllerId): keyof T =>
 export const outletConnected = ({ controllerId }: HasControllerId): string =>
   `${camelize(controllerId)}OutletConnected`
 
-export class LruCache<K, V> {
+export interface Cache<K, V> {
+  get(key: K): V | undefined
+  put(key: K, value: V): void
+}
+export class LruCache<K, V> implements Cache<K, V> {
   private values: Map<K, V> = new Map<K, V>()
   private maxEntries = 20
 

scripts/connected-users/test/users/api.test.ts

@@ -0,0 +1,146 @@
+import {
+  afterEach,
+  beforeEach,
+  describe,
+  jest,
+  expect,
+  test,
+} from '@jest/globals'
+
+import { UserFetcher } from '@connected-users/users/api'
+import { DeletedUser, ExistingUser, User } from '@connected-users/users/classes'
+
+async function* asAsyncIt<T>(arr: T[]): AsyncIterableIterator<T> {
+  yield* arr
+}
+const kebab = (value: string) =>
+  value
+    .replace(/([a-z])([A-Z])/g, '$1-$2')
+    .replace(/[\s_]+/g, '-')
+    .toLowerCase()
+
+const genUser = (id: number, name = 'User Name') => ({
+  user_id: id,
+  display_name: name,
+  badge_counts: { gold: 0, silver: 0, bronze: 0 },
+  link: `https://example.com/users/${id}/${kebab(name)}`,
+  profile_image: `https://img.example.com/${id}.png`,
+  reputation: 1,
+  user_type: 'registered',
+  is_employee: false,
+})
+
+describe('We can fetch users in batches from the Stack Exchange API', () => {
+  const MockedAPIClass = jest.createMockFromModule<
+    typeof import('@connected-users/seAPI')
+  >('@connected-users/seAPI').StackExchangeAPI
+  const mockedAPI = new MockedAPIClass()
+  const mockFetchAll = jest.mocked(mockedAPI.fetchAll)
+
+  const MockedCache = jest.createMockFromModule<
+    typeof import('@connected-users/utils')
+  >('@connected-users/utils').LruCache
+  const mockedCache = new MockedCache<User['user_id'], User>()
+  const mockCacheGet = jest.mocked(mockedCache.get)
+  const mockCachePut = jest.mocked(mockedCache.put)
+
+  let fetcher: UserFetcher
+
+  afterEach(() => {
+    jest.restoreAllMocks()
+  })
+  beforeEach(() => {
+    fetcher = UserFetcher.withDefaultClasses({
+      api: mockedAPI,
+      cache: mockedCache,
+    })
+  })
+
+  test('with an empty cache, all users are fetched from the API', async () => {
+    mockFetchAll.mockReturnValue(
+      asAsyncIt([1, 2, 3].map((id) => genUser(id, `User ${id}`)))
+    )
+    const gen = fetcher.users([1, 2, 3])
+    for await (const user of gen) {
+      expect(user).toBeInstanceOf(ExistingUser)
+      expect(mockCachePut).toHaveBeenCalledWith(user.user_id, user)
+    }
+    expect(mockFetchAll).toHaveBeenCalledWith(
+      '/users/{ids}',
+      { ids: [1, 2, 3] },
+      { filter: expect.any(String) }
+    )
+  })
+
+  test('cached users skip the API', async () => {
+    mockCacheGet.mockImplementation((id: number) =>
+      Object.assign(new ExistingUser(), genUser(id, `User ${id}`))
+    )
+    const gen = fetcher.users([1, 2, 3])
+    for await (const user of gen) {
+      expect(user).toBeInstanceOf(ExistingUser)
+    }
+    expect(mockFetchAll).not.toHaveBeenCalled()
+    expect(mockCachePut).not.toHaveBeenCalled()
+  })
+
+  test('mixing cached and uncached users produces users in requested order', async () => {
+    mockFetchAll.mockReturnValue(
+      asAsyncIt([2, 4].map((id) => genUser(id, `User ${id}`)))
+    )
+    mockCacheGet.mockImplementation((id: number) =>
+      [1, 3].includes(id)
+        ? Object.assign(new ExistingUser(), genUser(id, `User ${id}`))
+        : undefined
+    )
+
+    const gen = fetcher.users([1, 2, 3, 4])
+    let lastId = 0
+    for await (const user of gen) {
+      expect(user).toBeInstanceOf(ExistingUser)
+      expect(user.user_id).toBeGreaterThan(lastId)
+      lastId = user.user_id
+    }
+    expect(lastId).toBe(4)
+    expect(mockFetchAll).toHaveBeenCalledWith(
+      '/users/{ids}',
+      { ids: [2, 4] },
+      { filter: expect.any(String) }
+    )
+    expect(mockCachePut.mock.calls).toEqual([
+      [2, expect.any(ExistingUser)],
+      [4, expect.any(ExistingUser)],
+    ])
+  })
+
+  test('Missing user IDs are ignored ...', async () => {
+    mockFetchAll.mockReturnValue(
+      asAsyncIt([2, 4].map((id) => genUser(id, `User ${id}`)))
+    )
+    const gen = fetcher.users([1, 2, 3, 4])
+    const seen: number[] = []
+    for await (const user of gen) {
+      expect(user).toBeInstanceOf(ExistingUser)
+      seen.push(user.user_id)
+    }
+    expect(seen).toEqual([2, 4])
+  })
+
+  test('... unless we configure the fetcher to treat those as deleted', async () => {
+    mockFetchAll.mockReturnValue(
+      asAsyncIt([2, 4].map((id) => genUser(id, `User ${id}`)))
+    )
+    const fetcher = UserFetcher.withDefaultClasses({
+      api: mockedAPI,
+      cache: mockedCache,
+      missingAssumeDeleted: true,
+    })
+    const gen = fetcher.users([1, 2, 3, 4])
+    const seen: number[] = []
+    for await (const user of gen) {
+      expect(user).toBeInstanceOf(user.user_id % 2 ? DeletedUser : ExistingUser)
+      seen.push(user.user_id)
+    }
+    expect(seen).toEqual([1, 2, 3, 4])
+  })
+})