From d6e3c1c8275e73b7de5efaf1239439fd3369612d Mon Sep 17 00:00:00 2001 From: Jason Hartman Date: Mon, 18 Nov 2024 09:14:32 -0800 Subject: [PATCH] build(client-presence): type and lint fixes Minor production runtime changes with introduction of getOrCreateRecord helper and retyping of Object.entries/keys utilities. --- packages/framework/presence/.eslintrc.cjs | 1 - .../presence/api-report/presence.alpha.api.md | 2 +- .../presence/src/exposedInternalTypes.ts | 4 +- .../framework/presence/src/internalTypes.ts | 9 --- .../framework/presence/src/internalUtils.ts | 72 +++++++++++++++++ .../presence/src/latestMapValueManager.ts | 77 ++++++++++++------- .../presence/src/latestValueManager.ts | 4 +- .../presence/src/presenceDatastoreManager.ts | 13 ++-- .../framework/presence/src/presenceStates.ts | 20 +++-- 9 files changed, 145 insertions(+), 57 deletions(-) create mode 100644 packages/framework/presence/src/internalUtils.ts diff --git a/packages/framework/presence/.eslintrc.cjs b/packages/framework/presence/.eslintrc.cjs index 26db2e1351c7..3e874fdd454b 100644 --- a/packages/framework/presence/.eslintrc.cjs +++ b/packages/framework/presence/.eslintrc.cjs @@ -29,7 +29,6 @@ module.exports = { ], }, ], - "@fluid-internal/fluid/no-unchecked-record-access": "warn", }, overrides: [ { diff --git a/packages/framework/presence/api-report/presence.alpha.api.md b/packages/framework/presence/api-report/presence.alpha.api.md index 1ef5850b7cd7..e8dc24a542d7 100644 --- a/packages/framework/presence/api-report/presence.alpha.api.md +++ b/packages/framework/presence/api-report/presence.alpha.api.md @@ -58,7 +58,7 @@ export function Latest(initialVal // @alpha export function LatestMap(initialValues?: { [K in Keys]: JsonSerializable & JsonDeserialized; -}, controls?: BroadcastControlSettings): InternalTypes.ManagerFactory, LatestMapValueManager>; +}, controls?: BroadcastControlSettings): InternalTypes.ManagerFactory, LatestMapValueManager>; // @alpha @sealed export interface LatestMapItemRemovedClientData { diff --git a/packages/framework/presence/src/exposedInternalTypes.ts b/packages/framework/presence/src/exposedInternalTypes.ts index fd7dc2ae0b74..a571f1de7fe1 100644 --- a/packages/framework/presence/src/exposedInternalTypes.ts +++ b/packages/framework/presence/src/exposedInternalTypes.ts @@ -60,13 +60,13 @@ export namespace InternalTypes { /** * @system */ - export interface MapValueState { + export interface MapValueState { rev: number; items: { // Caution: any particular item may or may not exist // Typescript does not support absent keys without forcing type to also be undefined. // See https://github.com/microsoft/TypeScript/issues/42810. - [name: string | number]: ValueOptionalState; + [name in Keys]: ValueOptionalState; }; } diff --git a/packages/framework/presence/src/internalTypes.ts b/packages/framework/presence/src/internalTypes.ts index 756351d24eec..00d7b28bdcec 100644 --- a/packages/framework/presence/src/internalTypes.ts +++ b/packages/framework/presence/src/internalTypes.ts @@ -21,15 +21,6 @@ export interface ClientRecord( - o: Record, -) => [K, T][]; - /** * This interface is a subset of (IContainerRuntime & IRuntimeInternal) and * (IFluidDataStoreRuntime) that is needed by the Presence States. diff --git a/packages/framework/presence/src/internalUtils.ts b/packages/framework/presence/src/internalUtils.ts new file mode 100644 index 000000000000..3628d4662a4f --- /dev/null +++ b/packages/framework/presence/src/internalUtils.ts @@ -0,0 +1,72 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +/** + * Returns union of types of values in a record. + */ +export type RecordEntryTypes = T[keyof T]; + +type MapNumberIndicesToStrings = { + [K in keyof T as K extends number ? `${K}` : K]: T[K]; +}; + +type KeyValuePairs = { + [K in keyof MapNumberIndicesToStrings>]: [K, Required[K]]; +}[keyof MapNumberIndicesToStrings>][]; + +type RequiredAndNotUndefined = { + [K in keyof T]-?: Exclude; +}; + +/** + * Object.entries retyped to preserve known keys and their types. + * + * @internal + */ +export const objectEntries = Object.entries as (o: T) => KeyValuePairs; + +/** + * Object.entries retyped to preserve known keys and their types. + * + * @remarks + * Given `T` should not contain `undefined` values. If it does, use + * {@link objectEntries} instead. Without `undefined` values, this + * typing provides best handling of objects with optional properties. + * + * @internal + */ +export const objectEntriesWithoutUndefined = Object.entries as ( + o: T, +) => KeyValuePairs>; + +/** + * Object.keys retyped to preserve known keys and their types. + * + * @internal + */ +export const objectKeys = Object.keys as (o: T) => (keyof MapNumberIndicesToStrings)[]; + +/** + * Retrieve a value from a record with the given key, or create a new entry if + * the key is not in the record. + * + * @param record - The record to index/update + * @param key - The key to lookup in the record + * @param defaultValue - a function which returns a default value. This is + * called and used to set an initial value for the given key in the record if + * none exists. + * @returns either the existing value for the given key, or the newly-created + * value (the result of `defaultValue`) + */ +export function getOrCreateRecord( + record: Record, + key: K, + defaultValue: (key: K) => V, +): V { + if (!(key in record)) { + record[key] = defaultValue(key); + } + return record[key]; +} diff --git a/packages/framework/presence/src/latestMapValueManager.ts b/packages/framework/presence/src/latestMapValueManager.ts index 13c5992c4aba..ff9092cfb365 100644 --- a/packages/framework/presence/src/latestMapValueManager.ts +++ b/packages/framework/presence/src/latestMapValueManager.ts @@ -9,6 +9,7 @@ import type { Listenable } from "@fluidframework/core-interfaces"; import type { BroadcastControls, BroadcastControlSettings } from "./broadcastControls.js"; import { OptionalBroadcastControl } from "./broadcastControls.js"; import type { ValueManager } from "./internalTypes.js"; +import { objectEntries, objectKeys } from "./internalUtils.js"; import type { LatestValueClientData, LatestValueData, @@ -189,17 +190,28 @@ export interface ValueMap { class ValueMapImpl implements ValueMap { private countDefined: number; public constructor( - private readonly value: InternalTypes.MapValueState, - private readonly localUpdate: (updates: InternalTypes.MapValueState) => void, + private readonly value: InternalTypes.MapValueState, + private readonly localUpdate: ( + updates: InternalTypes.MapValueState< + T, + // This should be `K`, but will only work if properties are optional. + string | number + >, + ) => void, ) { // All initial items are expected to be defined. // TODO assert all defined and/or update type. this.countDefined = Object.keys(value.items).length; } + /** + * Note: caller must ensure key exists in this.value.items. + */ private updateItem(key: K, value: InternalTypes.ValueOptionalState["value"]): void { this.value.rev += 1; - const item = this.value.items[key]; + // Caller is required to ensure key exists. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const item = this.value.items[key]!; item.rev += 1; item.timestamp = Date.now(); if (value === undefined) { @@ -231,10 +243,9 @@ class ValueMapImpl implements ValueMap { ) => void, thisArg?: unknown, ): void { - for (const [key, item] of Object.entries(this.value.items)) { + for (const [key, item] of objectEntries(this.value.items)) { if (item.value !== undefined) { - // TODO: see about typing InternalTypes.MapValueState with K - callbackfn(item.value, key as K, this); + callbackfn(item.value, key, this); } } } @@ -257,9 +268,9 @@ class ValueMapImpl implements ValueMap { } public keys(): IterableIterator { const keys: K[] = []; - for (const [key, item] of Object.entries(this.value.items)) { + for (const [key, item] of objectEntries(this.value.items)) { if (item.value !== undefined) { - keys.push(key as K); + keys.push(key); } } return keys[Symbol.iterator](); @@ -312,7 +323,7 @@ class LatestMapValueManagerImpl< Keys extends string | number = string | number, > implements LatestMapValueManager, - Required>> + Required>> { public readonly events = createEmitter>(); public readonly controls: OptionalBroadcastControl; @@ -321,16 +332,16 @@ class LatestMapValueManagerImpl< private readonly key: RegistrationKey, private readonly datastore: StateDatastore< RegistrationKey, - InternalTypes.MapValueState + InternalTypes.MapValueState >, - public readonly value: InternalTypes.MapValueState, + public readonly value: InternalTypes.MapValueState, controlSettings: BroadcastControlSettings | undefined, ) { this.controls = new OptionalBroadcastControl(controlSettings); this.local = new ValueMapImpl( value, - (updates: InternalTypes.MapValueState) => { + (updates: InternalTypes.MapValueState) => { datastore.localUpdate(key, updates, { allowableUpdateLatencyMs: this.controls.allowableUpdateLatencyMs, }); @@ -342,7 +353,7 @@ class LatestMapValueManagerImpl< public *clientValues(): IterableIterator> { const allKnownStates = this.datastore.knownValues(this.key); - for (const clientSessionId of Object.keys(allKnownStates.states)) { + for (const clientSessionId of objectKeys(allKnownStates.states)) { if (clientSessionId !== allKnownStates.self) { const client = this.datastore.lookupClient(clientSessionId); const items = this.clientValue(client); @@ -353,7 +364,7 @@ class LatestMapValueManagerImpl< public clients(): ISessionClient[] { const allKnownStates = this.datastore.knownValues(this.key); - return Object.keys(allKnownStates.states) + return objectKeys(allKnownStates.states) .filter((clientSessionId) => clientSessionId !== allKnownStates.self) .map((clientSessionId) => this.datastore.lookupClient(clientSessionId)); } @@ -366,10 +377,10 @@ class LatestMapValueManagerImpl< } const clientStateMap = allKnownStates.states[clientSessionId]; const items = new Map>(); - for (const [key, item] of Object.entries(clientStateMap.items)) { + for (const [key, item] of objectEntries(clientStateMap.items)) { const value = item.value; if (value !== undefined) { - items.set(key as Keys, { + items.set(key, { value, metadata: { revision: item.rev, timestamp: item.timestamp }, }); @@ -381,20 +392,25 @@ class LatestMapValueManagerImpl< public update( client: SpecificSessionClient, _received: number, - value: InternalTypes.MapValueState, + value: InternalTypes.MapValueState, ): void { const allKnownStates = this.datastore.knownValues(this.key); const clientSessionId: SpecificSessionClientId = client.sessionId; if (!(clientSessionId in allKnownStates.states)) { // New client - prepare new client state directory - allKnownStates.states[clientSessionId] = { rev: value.rev, items: {} }; + allKnownStates.states[clientSessionId] = { + rev: value.rev, + items: {} as unknown as InternalTypes.MapValueState["items"], + }; } const currentState = allKnownStates.states[clientSessionId]; // Accumulate individual update keys const updatedItemKeys: Keys[] = []; - for (const [key, item] of Object.entries(value.items)) { - if (!(key in currentState.items) || currentState.items[key].rev < item.rev) { - updatedItemKeys.push(key as Keys); + for (const [key, item] of objectEntries(value.items)) { + // TODO: Key validation needs to be added here. + const validKey = key as Keys; + if (!(key in currentState.items) || currentState.items[validKey].rev < item.rev) { + updatedItemKeys.push(validKey); } } @@ -411,7 +427,8 @@ class LatestMapValueManagerImpl< items: new Map>(), }; for (const key of updatedItemKeys) { - const item = value.items[key]; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const item = value.items[key]!; const hadPriorValue = currentState.items[key]?.value; currentState.items[key] = item; const metadata = { revision: item.rev, timestamp: item.timestamp }; @@ -452,22 +469,26 @@ export function LatestMap< controls?: BroadcastControlSettings, ): InternalTypes.ManagerFactory< RegistrationKey, - InternalTypes.MapValueState, + InternalTypes.MapValueState, LatestMapValueManager > { const timestamp = Date.now(); - const value: InternalTypes.MapValueState = { rev: 0, items: {} }; + const value: InternalTypes.MapValueState< + T, + // This should be `Keys`, but will only work if properties are optional. + string | number + > = { rev: 0, items: {} }; // LatestMapValueManager takes ownership of values within initialValues. if (initialValues !== undefined) { - for (const key of Object.keys(initialValues)) { - value.items[key] = { rev: 0, timestamp, value: initialValues[key as Keys] }; + for (const key of objectKeys(initialValues)) { + value.items[key] = { rev: 0, timestamp, value: initialValues[key] }; } } const factory = ( key: RegistrationKey, datastoreHandle: InternalTypes.StateDatastoreHandle< RegistrationKey, - InternalTypes.MapValueState + InternalTypes.MapValueState >, ): { initialData: { value: typeof value; allowableUpdateLatencyMs: number | undefined }; @@ -477,7 +498,7 @@ export function LatestMap< manager: brandIVM< LatestMapValueManagerImpl, T, - InternalTypes.MapValueState + InternalTypes.MapValueState >( new LatestMapValueManagerImpl( key, diff --git a/packages/framework/presence/src/latestValueManager.ts b/packages/framework/presence/src/latestValueManager.ts index 6d2c73fbe94d..0c4c22e1d486 100644 --- a/packages/framework/presence/src/latestValueManager.ts +++ b/packages/framework/presence/src/latestValueManager.ts @@ -9,7 +9,7 @@ import type { Listenable } from "@fluidframework/core-interfaces"; import type { BroadcastControls, BroadcastControlSettings } from "./broadcastControls.js"; import { OptionalBroadcastControl } from "./broadcastControls.js"; import type { ValueManager } from "./internalTypes.js"; -import { brandedObjectEntries } from "./internalTypes.js"; +import { objectEntries } from "./internalUtils.js"; import type { LatestValueClientData, LatestValueData } from "./latestValueTypes.js"; import type { ISessionClient } from "./presence.js"; import { datastoreFromHandle, type StateDatastore } from "./stateDatastore.js"; @@ -110,7 +110,7 @@ class LatestValueManagerImpl public *clientValues(): IterableIterator> { const allKnownStates = this.datastore.knownValues(this.key); - for (const [clientSessionId, value] of brandedObjectEntries(allKnownStates.states)) { + for (const [clientSessionId, value] of objectEntries(allKnownStates.states)) { if (clientSessionId !== allKnownStates.self) { yield { client: this.datastore.lookupClient(clientSessionId), diff --git a/packages/framework/presence/src/presenceDatastoreManager.ts b/packages/framework/presence/src/presenceDatastoreManager.ts index e5140fb9e568..1e17598118ed 100644 --- a/packages/framework/presence/src/presenceDatastoreManager.ts +++ b/packages/framework/presence/src/presenceDatastoreManager.ts @@ -9,8 +9,8 @@ import type { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils/intern import type { ClientConnectionId } from "./baseTypes.js"; import type { BroadcastControlSettings } from "./broadcastControls.js"; -import { brandedObjectEntries } from "./internalTypes.js"; import type { IEphemeralRuntime } from "./internalTypes.js"; +import { objectEntries } from "./internalUtils.js"; import type { ClientSessionId, ISessionClient } from "./presence.js"; import type { ClientUpdateEntry, @@ -116,9 +116,7 @@ function mergeGeneralDatastoreMessageContent( // Iterate over each value manager and its data, merging it as needed. for (const valueManagerKey of Object.keys(workspaceData)) { - for (const [clientSessionId, value] of brandedObjectEntries( - workspaceData[valueManagerKey], - )) { + for (const [clientSessionId, value] of objectEntries(workspaceData[valueManagerKey])) { mergedData[valueManagerKey] ??= {}; const oldData = mergedData[valueManagerKey][clientSessionId]; mergedData[valueManagerKey][clientSessionId] = mergeValueDirectory( @@ -191,7 +189,8 @@ export class PresenceDatastoreManagerImpl implements PresenceDatastoreManager { return existing.internal.ensureContent(requestedContent, controls); } - let workspaceDatastore = this.datastore[internalWorkspaceAddress]; + let workspaceDatastore: ValueElementMap | undefined = + this.datastore[internalWorkspaceAddress]; if (workspaceDatastore === undefined) { workspaceDatastore = this.datastore[internalWorkspaceAddress] = {}; } @@ -302,7 +301,9 @@ export class PresenceDatastoreManagerImpl implements PresenceDatastoreManager { const clientConnectionId = this.runtime.clientId; assert(clientConnectionId !== undefined, 0xa59 /* Client connected without clientId */); const currentClientToSessionValueState = - this.datastore["system:presence"].clientToSessionId[clientConnectionId]; + // When connected, `clientToSessionId` must always have current connection entry. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + this.datastore["system:presence"].clientToSessionId[clientConnectionId]!; const newMessage = { sendTimestamp: Date.now(), diff --git a/packages/framework/presence/src/presenceStates.ts b/packages/framework/presence/src/presenceStates.ts index d3e492d3e4fb..fde0860b7082 100644 --- a/packages/framework/presence/src/presenceStates.ts +++ b/packages/framework/presence/src/presenceStates.ts @@ -10,7 +10,8 @@ import type { BroadcastControlSettings } from "./broadcastControls.js"; import { RequiredBroadcastControl } from "./broadcastControls.js"; import type { InternalTypes } from "./exposedInternalTypes.js"; import type { ClientRecord } from "./internalTypes.js"; -import { brandedObjectEntries } from "./internalTypes.js"; +import type { RecordEntryTypes } from "./internalUtils.js"; +import { getOrCreateRecord, objectEntries } from "./internalUtils.js"; import type { ClientSessionId, ISessionClient } from "./presence.js"; import type { LocalStateUpdateOptions, StateDatastore } from "./stateDatastore.js"; import { handleFromDatastore } from "./stateDatastore.js"; @@ -211,11 +212,12 @@ export function mergeUntrackedDatastore( datastore: ValueElementMap, timeModifier: number, ): void { - if (!(key in datastore)) { - datastore[key] = {}; - } - const localAllKnownState = datastore[key]; - for (const [clientSessionId, value] of brandedObjectEntries(remoteAllKnownState)) { + const localAllKnownState = getOrCreateRecord( + datastore, + key, + (): RecordEntryTypes => ({}), + ); + for (const [clientSessionId, value] of objectEntries(remoteAllKnownState)) { if (!("ignoreUnmonitored" in value)) { localAllKnownState[clientSessionId] = mergeValueDirectory( localAllKnownState[clientSessionId], @@ -344,7 +346,9 @@ class PresenceStatesImpl clientId: ClientSessionId, value: Exclude, undefined>["value"], ): void { - const allKnownState = this.datastore[key]; + // Callers my only use `key`s that are part of `this.datastore`. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const allKnownState = this.datastore[key]!; allKnownState[clientId] = mergeValueDirectory(allKnownState[clientId], value, 0); } @@ -412,7 +416,7 @@ class PresenceStatesImpl for (const [key, remoteAllKnownState] of Object.entries(remoteDatastore)) { if (key in this.nodes) { const node = unbrandIVM(this.nodes[key]); - for (const [clientSessionId, value] of brandedObjectEntries(remoteAllKnownState)) { + for (const [clientSessionId, value] of objectEntries(remoteAllKnownState)) { const client = this.runtime.lookupClient(clientSessionId); node.update(client, received, value); }