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.
microsoft · May 28, 2024 · 68e5eb3 · 68e5eb3
1 parent 936f247
commit 68e5eb3
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 36 deletions.
diff --git 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<any> {
 		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<string, unknown>) => 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<T = any>(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
@@ -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<IDirectoryEvents> 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<IDirectoryEvents> 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
@@ -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;
 		}