From 68e5eb35c97d71aaab472908fcd7cb56612f172b Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Tue, 28 May 2024 14:32:54 -0700 Subject: [PATCH] Remove extra calls to bind when not attached (#21213) With the change to the ordering of getting the attachment summary in #20998 and the testing in #20995 and #21132, we can now remove the workarounds that call bindHandles or makeSerializable when not in an attached state. --- .../dds/attributable-map/src/mapKernel.ts | 24 +------------------ packages/dds/map/src/directory.ts | 17 ++++++------- packages/dds/map/src/mapKernel.ts | 6 +---- 3 files changed, 11 insertions(+), 36 deletions(-) diff --git a/experimental/dds/attributable-map/src/mapKernel.ts b/experimental/dds/attributable-map/src/mapKernel.ts index a5a698342470..3814ff86508a 100644 --- a/experimental/dds/attributable-map/src/mapKernel.ts +++ b/experimental/dds/attributable-map/src/mapKernel.ts @@ -8,11 +8,7 @@ import { IFluidHandle } from "@fluidframework/core-interfaces"; import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; import { ISequencedDocumentMessage } from "@fluidframework/driver-definitions"; import { AttributionKey } from "@fluidframework/runtime-definitions/internal"; -import { - IFluidSerializer, - ValueType, - bindHandles, -} from "@fluidframework/shared-object-base/internal"; +import { IFluidSerializer, ValueType } from "@fluidframework/shared-object-base/internal"; // eslint-disable-next-line import/no-deprecated import { ISerializableValue, ISerializedValue, ISharedMapEvents } from "./interfaces.js"; @@ -84,8 +80,6 @@ export interface IMapDataObjectSerialized { type MapKeyLocalOpMetadata = IMapKeyEditLocalOpMetadata | IMapKeyAddLocalOpMetadata; type MapLocalOpMetadata = IMapClearLocalOpMetadata | MapKeyLocalOpMetadata; -/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access */ - function isMapKeyLocalOpMetadata(metadata: any): metadata is MapKeyLocalOpMetadata { return ( metadata !== undefined && @@ -110,8 +104,6 @@ function isMapLocalOpMetadata(metadata: any): metadata is MapLocalOpMetadata { ); } -/* eslint-enable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access */ - function createClearLocalOpMetadata( op: IMapClearOperation, pendingClearMessageId: number, @@ -216,7 +208,6 @@ export class AttributableMapKernel { * @returns The iterator */ // TODO: Use `unknown` instead (breaking change). - // eslint-disable-next-line @typescript-eslint/no-explicit-any public entries(): IterableIterator<[string, any]> { const localEntriesIterator = this.data.entries(); const iterator = { @@ -239,7 +230,6 @@ export class AttributableMapKernel { * @returns The iterator */ // TODO: Use `unknown` instead (breaking change). - // eslint-disable-next-line @typescript-eslint/no-explicit-any public values(): IterableIterator { const localValuesIterator = this.data.values(); const iterator = { @@ -262,7 +252,6 @@ export class AttributableMapKernel { * @returns The iterator */ // TODO: Use `unknown` instead (breaking change). - // eslint-disable-next-line @typescript-eslint/no-explicit-any public [Symbol.iterator](): IterableIterator<[string, any]> { return this.entries(); } @@ -274,7 +263,6 @@ export class AttributableMapKernel { public forEach( callbackFn: (value: unknown, key: string, map: Map) => void, ): void { - // eslint-disable-next-line unicorn/no-array-for-each this.data.forEach((localValue, key, m) => { callbackFn(localValue.value, key, m); }); @@ -284,7 +272,6 @@ export class AttributableMapKernel { * {@inheritDoc ISharedMap.get} */ // TODO: Use `unknown` instead (breaking change). - // eslint-disable-next-line @typescript-eslint/no-explicit-any public get(key: string): T | undefined { const localValue = this.data.get(key); return localValue === undefined ? undefined : (localValue.value as T); @@ -317,10 +304,6 @@ export class AttributableMapKernel { // If we are not attached, don't submit the op. if (!this.isAttached()) { - // this is necessary to bind the potential handles in the value - // to this DDS, as we do not walk the object normally unless we - // are attached - bindHandles(localValue.value, this.serializer, this.handle); return; } @@ -546,14 +529,11 @@ export class AttributableMapKernel { return true; } - /* eslint-disable @typescript-eslint/no-unsafe-member-access */ - /** * Rollback a local op * @param op - The operation to rollback * @param localOpMetadata - The local metadata associated with the op. */ - // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/no-explicit-any public rollback(op: any, localOpMetadata: unknown): void { if (!isMapLocalOpMetadata(localOpMetadata)) { throw new Error("Invalid localOpMetadata"); @@ -599,8 +579,6 @@ export class AttributableMapKernel { } } - /* eslint-enable @typescript-eslint/no-unsafe-member-access */ - /** * Set implementation used for both locally sourced sets as well as incoming remote sets. * @param key - The key being set diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index 2ee82fdf0315..6e64bce2022f 100644 --- a/packages/dds/map/src/directory.ts +++ b/packages/dds/map/src/directory.ts @@ -20,7 +20,12 @@ import type { } from "@fluidframework/runtime-definitions/internal"; import { SummaryTreeBuilder } from "@fluidframework/runtime-utils/internal"; import type { IFluidSerializer } from "@fluidframework/shared-object-base/internal"; -import { SharedObject, ValueType, parseHandles } from "@fluidframework/shared-object-base/internal"; +import { + SharedObject, + ValueType, + bindHandles, + parseHandles, +} from "@fluidframework/shared-object-base/internal"; import { type ITelemetryLoggerExt, UsageError } from "@fluidframework/telemetry-utils/internal"; import path from "path-browserify"; @@ -38,7 +43,7 @@ import type { ISerializedValue, } from "./internalInterfaces.js"; import type { ILocalValue } from "./localValues.js"; -import { LocalValueMaker, makeSerializable } from "./localValues.js"; +import { LocalValueMaker } from "./localValues.js"; // We use path-browserify since this code can run safely on the server or the browser. // We standardize on using posix slashes everywhere. @@ -1307,11 +1312,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec // Create a local value and serialize it. const localValue = this.directory.localValueMaker.fromInMemory(value); - const serializableValue = makeSerializable( - localValue, - this.serializer, - this.directory.handle, - ); + bindHandles(localValue, this.serializer, this.directory.handle); // Set the value locally. const previousValue = this.setCore(key, localValue, true); @@ -1325,7 +1326,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec key, path: this.absolutePath, type: "set", - value: serializableValue, + value: { type: localValue.type, value: localValue.value as unknown }, }; this.submitKeyMessage(op, previousValue); return this; diff --git a/packages/dds/map/src/mapKernel.ts b/packages/dds/map/src/mapKernel.ts index ce9567b6cc13..8cf921b2e467 100644 --- a/packages/dds/map/src/mapKernel.ts +++ b/packages/dds/map/src/mapKernel.ts @@ -7,7 +7,7 @@ import type { TypedEventEmitter } from "@fluid-internal/client-utils"; import type { IFluidHandle } from "@fluidframework/core-interfaces"; import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; import type { IFluidSerializer } from "@fluidframework/shared-object-base/internal"; -import { ValueType, bindHandles } from "@fluidframework/shared-object-base/internal"; +import { ValueType } from "@fluidframework/shared-object-base/internal"; import type { ISharedMapEvents } from "./interfaces.js"; import type { @@ -299,10 +299,6 @@ export class MapKernel { // If we are not attached, don't submit the op. if (!this.isAttached()) { - // this is necessary to bind the potential handles in the value - // to this DDS, as we do not walk the object normally unless we - // are attached - bindHandles(localValue.value, this.serializer, this.handle); return; }