From c92cc613605b9447067ce8242365dd1e6184be89 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 23 May 2024 00:13:02 +0000 Subject: [PATCH 1/8] Remove extra calls to bind when not attached --- packages/dds/map/src/directory.ts | 9 ++------- packages/dds/map/src/mapKernel.ts | 6 +----- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index 6db03f3a0825..faa52bcfb440 100644 --- a/packages/dds/map/src/directory.ts +++ b/packages/dds/map/src/directory.ts @@ -37,7 +37,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. @@ -1306,11 +1306,6 @@ 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, - ); // Set the value locally. const previousValue = this.setCore(key, localValue, true); @@ -1324,7 +1319,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; } From 5dec82a19de9a7e4f7e2b333be9a3b7637f4d094 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Wed, 22 May 2024 17:16:58 -0700 Subject: [PATCH 2/8] Update packages/dds/map/src/directory.ts --- packages/dds/map/src/directory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index faa52bcfb440..5823b33fdccd 100644 --- a/packages/dds/map/src/directory.ts +++ b/packages/dds/map/src/directory.ts @@ -1304,7 +1304,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec throw new Error("Undefined and null keys are not supported"); } - // Create a local value and serialize it. + // Create a local value. const localValue = this.directory.localValueMaker.fromInMemory(value); // Set the value locally. From b1613a1437a2a4d9b16477621715ec21bb11c762 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 23 May 2024 17:27:08 +0000 Subject: [PATCH 3/8] Remove bindHandles from attributablemap --- .../dds/attributable-map/src/mapKernel.ts | 24 +------------------ packages/dds/shared-object-base/src/index.ts | 1 - packages/dds/shared-object-base/src/utils.ts | 13 ---------- 3 files changed, 1 insertion(+), 37 deletions(-) diff --git a/experimental/dds/attributable-map/src/mapKernel.ts b/experimental/dds/attributable-map/src/mapKernel.ts index 05ddb0e8e0bd..e09556c1af7a 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/protocol-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/shared-object-base/src/index.ts b/packages/dds/shared-object-base/src/index.ts index 551689f46b42..e4b76eba45ff 100644 --- a/packages/dds/shared-object-base/src/index.ts +++ b/packages/dds/shared-object-base/src/index.ts @@ -18,6 +18,5 @@ export { makeHandlesSerializable, parseHandles, serializeHandles, - bindHandles, } from "./utils.js"; export { ValueType } from "./valueType.js"; diff --git a/packages/dds/shared-object-base/src/utils.ts b/packages/dds/shared-object-base/src/utils.ts index 87c633cc1bc0..01cac9cc6264 100644 --- a/packages/dds/shared-object-base/src/utils.ts +++ b/packages/dds/shared-object-base/src/utils.ts @@ -80,16 +80,3 @@ export function createSingleBlobSummary( builder.addBlob(key, content); return builder.getSummaryTree(); } - -/** - * Binds all handles found in `value` to `bind`. Does not modify original input. - * - * @internal - */ -export function bindHandles(value: any, serializer: IFluidSerializer, bind: IFluidHandle): void { - // N.B. AB#7316 this could be made more efficient by writing an ad hoc - // implementation that doesn't clone at all. Today the distinction between - // this function and `encode` is purely semantic -- encoding both serializes - // handles and binds them, but sometimes we only wish to do the latter - serializer.encode(value, bind); -} From 53a9f6619c190daf51ee0a28fcdfcf29462a67f4 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 23 May 2024 18:50:31 +0000 Subject: [PATCH 4/8] Fix type tests --- .../api-report/shared-object-base.api.md | 3 --- packages/dds/shared-object-base/package.json | 7 ++++++- ...lidateSharedObjectBasePrevious.generated.ts | 18 +++--------------- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/packages/dds/shared-object-base/api-report/shared-object-base.api.md b/packages/dds/shared-object-base/api-report/shared-object-base.api.md index 94f616f4d331..99c9e3b994ff 100644 --- a/packages/dds/shared-object-base/api-report/shared-object-base.api.md +++ b/packages/dds/shared-object-base/api-report/shared-object-base.api.md @@ -28,9 +28,6 @@ import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions/inter import { ITelemetryContext } from '@fluidframework/runtime-definitions/internal'; import { ITelemetryLoggerExt } from '@fluidframework/telemetry-utils/internal'; -// @internal -export function bindHandles(value: any, serializer: IFluidSerializer, bind: IFluidHandle): void; - // @internal export function createSharedObjectKind(factory: (new () => IChannelFactory) & { readonly Type: string; diff --git a/packages/dds/shared-object-base/package.json b/packages/dds/shared-object-base/package.json index dd3950c2fc50..3df2125619d9 100644 --- a/packages/dds/shared-object-base/package.json +++ b/packages/dds/shared-object-base/package.json @@ -147,6 +147,11 @@ "typescript": "~5.3.3" }, "typeValidation": { - "broken": {} + "broken": { + "RemovedFunctionDeclaration_bindHandles": { + "forwardCompat": false, + "backCompat": false + } + } } } diff --git a/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts b/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts index f01155068ae4..4f18904166cb 100644 --- a/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts +++ b/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts @@ -298,28 +298,16 @@ use_old_EnumDeclaration_ValueType( * If this test starts failing, it indicates a change that is not forward compatible. * To acknowledge the breaking change, add the following to package.json under * typeValidation.broken: - * "FunctionDeclaration_bindHandles": {"forwardCompat": false} + * "RemovedFunctionDeclaration_bindHandles": {"forwardCompat": false} */ -declare function get_old_FunctionDeclaration_bindHandles(): - TypeOnly; -declare function use_current_FunctionDeclaration_bindHandles( - use: TypeOnly): void; -use_current_FunctionDeclaration_bindHandles( - get_old_FunctionDeclaration_bindHandles()); /* * Validate backward compatibility by using the current type in place of the old type. * If this test starts failing, it indicates a change that is not backward compatible. * To acknowledge the breaking change, add the following to package.json under * typeValidation.broken: - * "FunctionDeclaration_bindHandles": {"backCompat": false} - */ -declare function get_current_FunctionDeclaration_bindHandles(): - TypeOnly; -declare function use_old_FunctionDeclaration_bindHandles( - use: TypeOnly): void; -use_old_FunctionDeclaration_bindHandles( - get_current_FunctionDeclaration_bindHandles()); + * "RemovedFunctionDeclaration_bindHandles": {"backCompat": false} + */ /* * Validate forward compatibility by using the old type in place of the current type. From 3775af7ed513f1e348d6e53780b3267e16c6ed9f Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 23 May 2024 20:29:16 +0000 Subject: [PATCH 5/8] Revert changes to directory --- packages/dds/map/src/directory.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index 838f18302b10..2ee82fdf0315 100644 --- a/packages/dds/map/src/directory.ts +++ b/packages/dds/map/src/directory.ts @@ -38,7 +38,7 @@ import type { ISerializedValue, } from "./internalInterfaces.js"; import type { ILocalValue } from "./localValues.js"; -import { LocalValueMaker } from "./localValues.js"; +import { LocalValueMaker, makeSerializable } 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. @@ -1305,8 +1305,13 @@ class SubDirectory extends TypedEventEmitter implements IDirec throw new Error("Undefined and null keys are not supported"); } - // Create a local value. + // Create a local value and serialize it. const localValue = this.directory.localValueMaker.fromInMemory(value); + const serializableValue = makeSerializable( + localValue, + this.serializer, + this.directory.handle, + ); // Set the value locally. const previousValue = this.setCore(key, localValue, true); @@ -1320,7 +1325,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec key, path: this.absolutePath, type: "set", - value: { type: localValue.type, value: localValue.value as unknown }, + value: serializableValue, }; this.submitKeyMessage(op, previousValue); return this; From ca1c5ca1c4f5563e7a578659a8cb1d28440df8d7 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Tue, 28 May 2024 19:07:52 +0000 Subject: [PATCH 6/8] Update directory.set to use bindHandles --- packages/dds/map/src/directory.ts | 8 ++++---- packages/dds/shared-object-base/package.json | 7 +------ packages/dds/shared-object-base/src/index.ts | 1 + ...lidateSharedObjectBasePrevious.generated.ts | 18 +++++++++++++++--- packages/dds/shared-object-base/src/utils.ts | 13 +++++++++++++ 5 files changed, 34 insertions(+), 13 deletions(-) diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index 2ee82fdf0315..453c81fe179f 100644 --- a/packages/dds/map/src/directory.ts +++ b/packages/dds/map/src/directory.ts @@ -20,7 +20,7 @@ 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 +38,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,7 +1307,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec // Create a local value and serialize it. const localValue = this.directory.localValueMaker.fromInMemory(value); - const serializableValue = makeSerializable( + bindHandles( localValue, this.serializer, this.directory.handle, @@ -1325,7 +1325,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/shared-object-base/package.json b/packages/dds/shared-object-base/package.json index 3df2125619d9..dd3950c2fc50 100644 --- a/packages/dds/shared-object-base/package.json +++ b/packages/dds/shared-object-base/package.json @@ -147,11 +147,6 @@ "typescript": "~5.3.3" }, "typeValidation": { - "broken": { - "RemovedFunctionDeclaration_bindHandles": { - "forwardCompat": false, - "backCompat": false - } - } + "broken": {} } } diff --git a/packages/dds/shared-object-base/src/index.ts b/packages/dds/shared-object-base/src/index.ts index e4b76eba45ff..551689f46b42 100644 --- a/packages/dds/shared-object-base/src/index.ts +++ b/packages/dds/shared-object-base/src/index.ts @@ -18,5 +18,6 @@ export { makeHandlesSerializable, parseHandles, serializeHandles, + bindHandles, } from "./utils.js"; export { ValueType } from "./valueType.js"; diff --git a/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts b/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts index 4f18904166cb..f01155068ae4 100644 --- a/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts +++ b/packages/dds/shared-object-base/src/test/types/validateSharedObjectBasePrevious.generated.ts @@ -298,16 +298,28 @@ use_old_EnumDeclaration_ValueType( * If this test starts failing, it indicates a change that is not forward compatible. * To acknowledge the breaking change, add the following to package.json under * typeValidation.broken: - * "RemovedFunctionDeclaration_bindHandles": {"forwardCompat": false} + * "FunctionDeclaration_bindHandles": {"forwardCompat": false} */ +declare function get_old_FunctionDeclaration_bindHandles(): + TypeOnly; +declare function use_current_FunctionDeclaration_bindHandles( + use: TypeOnly): void; +use_current_FunctionDeclaration_bindHandles( + get_old_FunctionDeclaration_bindHandles()); /* * Validate backward compatibility by using the current type in place of the old type. * If this test starts failing, it indicates a change that is not backward compatible. * To acknowledge the breaking change, add the following to package.json under * typeValidation.broken: - * "RemovedFunctionDeclaration_bindHandles": {"backCompat": false} - */ + * "FunctionDeclaration_bindHandles": {"backCompat": false} + */ +declare function get_current_FunctionDeclaration_bindHandles(): + TypeOnly; +declare function use_old_FunctionDeclaration_bindHandles( + use: TypeOnly): void; +use_old_FunctionDeclaration_bindHandles( + get_current_FunctionDeclaration_bindHandles()); /* * Validate forward compatibility by using the old type in place of the current type. diff --git a/packages/dds/shared-object-base/src/utils.ts b/packages/dds/shared-object-base/src/utils.ts index 01cac9cc6264..87c633cc1bc0 100644 --- a/packages/dds/shared-object-base/src/utils.ts +++ b/packages/dds/shared-object-base/src/utils.ts @@ -80,3 +80,16 @@ export function createSingleBlobSummary( builder.addBlob(key, content); return builder.getSummaryTree(); } + +/** + * Binds all handles found in `value` to `bind`. Does not modify original input. + * + * @internal + */ +export function bindHandles(value: any, serializer: IFluidSerializer, bind: IFluidHandle): void { + // N.B. AB#7316 this could be made more efficient by writing an ad hoc + // implementation that doesn't clone at all. Today the distinction between + // this function and `encode` is purely semantic -- encoding both serializes + // handles and binds them, but sometimes we only wish to do the latter + serializer.encode(value, bind); +} From 0d39ac973d173de782a052ed197ba9eb7801dddb Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Tue, 28 May 2024 20:28:38 +0000 Subject: [PATCH 7/8] prettier --- packages/dds/map/src/directory.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index 453c81fe179f..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, bindHandles, 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"; @@ -1307,11 +1312,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec // Create a local value and serialize it. const localValue = this.directory.localValueMaker.fromInMemory(value); - bindHandles( - localValue, - this.serializer, - this.directory.handle, - ); + bindHandles(localValue, this.serializer, this.directory.handle); // Set the value locally. const previousValue = this.setCore(key, localValue, true); From 6040a46c5be77095da3e6e375d039633111e5edc Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Tue, 28 May 2024 20:44:45 +0000 Subject: [PATCH 8/8] Revert api changes --- .../shared-object-base/api-report/shared-object-base.api.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/dds/shared-object-base/api-report/shared-object-base.api.md b/packages/dds/shared-object-base/api-report/shared-object-base.api.md index 99c9e3b994ff..94f616f4d331 100644 --- a/packages/dds/shared-object-base/api-report/shared-object-base.api.md +++ b/packages/dds/shared-object-base/api-report/shared-object-base.api.md @@ -28,6 +28,9 @@ import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions/inter import { ITelemetryContext } from '@fluidframework/runtime-definitions/internal'; import { ITelemetryLoggerExt } from '@fluidframework/telemetry-utils/internal'; +// @internal +export function bindHandles(value: any, serializer: IFluidSerializer, bind: IFluidHandle): void; + // @internal export function createSharedObjectKind(factory: (new () => IChannelFactory) & { readonly Type: string;