Skip to content

Commit

Permalink
Expose .is() typeguard on SharedObjectKind (#21850)
Browse files Browse the repository at this point in the history
## Description

With the removal of concrete DDS classes, one pattern that's come up
repeatedly is customer code which previously checked `instanceof
MySharedObject` (usually when the code supported multiple shared object
types for whatever reason), which no longer works. This change adds a
drop-in replacement to the public API surface.

## Breaking Changes

As `SharedObjectKind` was marked sealed, this is non-breaking.


## Alternatives Considered

We could expose free functions in each package easily e.g. by using a
helper like this:

```typescript
export function createSharedObjectTypeguard<TSharedObject>(
	kind: ISharedObjectKind<TSharedObject>,
): (loadable: IFluidLoadable) => loadable is IFluidLoadable & TSharedObject {
	const factoryType = kind.getFactory().type;
	return (loadable: IFluidLoadable): loadable is IFluidLoadable & TSharedObject => {
		return isChannel(loadable) && loadable.attributes.type === factoryType;
	};
}
```

Ultimately this will be more code though and arguably less discoverable.

We could also add back support for `instanceof` using
`Symbol.hasInstance` (and the same implementation as `.is`), but due to
microsoft/TypeScript#56536, this won't work
for customers using TS below 5.5, so we'll need something else anyway at
least for now.

---------

Co-authored-by: Abram Sanderson <[email protected]>
Co-authored-by: Craig Macomber (Microsoft) <[email protected]>
  • Loading branch information
3 people authored Jul 16, 2024
1 parent 3efdd9e commit 6bdec1a
Show file tree
Hide file tree
Showing 31 changed files with 200 additions and 15 deletions.
23 changes: 23 additions & 0 deletions .changeset/gold-badgers-float.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
"@fluidframework/shared-object-base": minor
---

Added typeguard for SharedObjectKinds

In the 2.0 release of Fluid, the concrete class implementations for DDSes were hidden from Fluid's API surface.
This made `instanceof` checks fail to work correctly.
There were ways to work around this in application code, but they involved boilerplate which required more understanding of Fluid internals than should be necessary.
This change adds a drop-in replacement to `instanceof`: the `.is()` method to `SharedObjectKind`.
For example:

```typescript
// Works in FluidFramework 1.0 but not in the initial release of FluidFramework 2.0:
if (myObject instanceof SharedString) {
// do something
}

// With this change, that code can now be written like so:
if (SharedString.is(myObject)) {
// do something
}
```
6 changes: 5 additions & 1 deletion packages/dds/cell/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_SharedCell": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ declare type current_as_old_for_InterfaceDeclaration_ISharedCellEvents = require
* typeValidation.broken:
* "VariableDeclaration_SharedCell": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedCell = requireAssignableTo<TypeOnly<typeof old.SharedCell>, TypeOnly<typeof current.SharedCell>>

/*
Expand Down
6 changes: 5 additions & 1 deletion packages/dds/counter/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_SharedCounter": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ declare type current_as_old_for_InterfaceDeclaration_ISharedCounterEvents = requ
* typeValidation.broken:
* "VariableDeclaration_SharedCounter": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedCounter = requireAssignableTo<TypeOnly<typeof old.SharedCounter>, TypeOnly<typeof current.SharedCounter>>

/*
Expand Down
9 changes: 8 additions & 1 deletion packages/dds/map/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,13 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_SharedMap": {
"forwardCompat": false
},
"VariableDeclaration_SharedDirectory": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ declare type current_as_old_for_ClassDeclaration_MapFactory = requireAssignableT
* typeValidation.broken:
* "VariableDeclaration_SharedDirectory": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedDirectory = requireAssignableTo<TypeOnly<typeof old.SharedDirectory>, TypeOnly<typeof current.SharedDirectory>>

/*
Expand Down Expand Up @@ -265,6 +266,7 @@ declare type current_as_old_for_TypeAliasDeclaration_SharedDirectory = requireAs
* typeValidation.broken:
* "VariableDeclaration_SharedMap": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedMap = requireAssignableTo<TypeOnly<typeof old.SharedMap>, TypeOnly<typeof current.SharedMap>>

/*
Expand Down
6 changes: 5 additions & 1 deletion packages/dds/matrix/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_SharedMatrix": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ declare type current_as_old_for_TypeAliasDeclaration_MatrixItem = requireAssigna
* typeValidation.broken:
* "VariableDeclaration_SharedMatrix": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedMatrix = requireAssignableTo<TypeOnly<typeof old.SharedMatrix>, TypeOnly<typeof current.SharedMatrix>>

/*
Expand Down
6 changes: 5 additions & 1 deletion packages/dds/ordered-collection/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_ConsensusQueue": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ declare type current_as_old_for_ClassDeclaration_ConsensusOrderedCollection = re
* typeValidation.broken:
* "VariableDeclaration_ConsensusQueue": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_ConsensusQueue = requireAssignableTo<TypeOnly<typeof old.ConsensusQueue>, TypeOnly<typeof current.ConsensusQueue>>

/*
Expand Down
6 changes: 5 additions & 1 deletion packages/dds/register-collection/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_ConsensusRegisterCollection": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ declare type MakeUnusedImportErrorsGoAway<T> = TypeOnly<T> | MinimalType<T> | Fu
* typeValidation.broken:
* "VariableDeclaration_ConsensusRegisterCollection": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_ConsensusRegisterCollection = requireAssignableTo<TypeOnly<typeof old.ConsensusRegisterCollection>, TypeOnly<typeof current.ConsensusRegisterCollection>>

/*
Expand Down
3 changes: 3 additions & 0 deletions packages/dds/sequence/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,9 @@
"broken": {
"InterfaceDeclaration_SequenceOptions": {
"backCompat": false
},
"VariableDeclaration_SharedString": {
"forwardCompat": false
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,7 @@ declare type current_as_old_for_ClassDeclaration_SharedSequence = requireAssigna
* typeValidation.broken:
* "VariableDeclaration_SharedString": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedString = requireAssignableTo<TypeOnly<typeof old.SharedString>, TypeOnly<typeof current.SharedString>>

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

// @public @sealed
export interface SharedObjectKind<out TSharedObject = unknown> extends ErasedType<readonly ["SharedObjectKind", TSharedObject]> {
is(value: IFluidLoadable): value is IFluidLoadable & TSharedObject;
}

// (No @packageDocumentation comment for this package)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export abstract class SharedObjectCore<TEvent extends ISharedObjectEvents = ISha

// @public @sealed
export interface SharedObjectKind<out TSharedObject = unknown> extends ErasedType<readonly ["SharedObjectKind", TSharedObject]> {
is(value: IFluidLoadable): value is IFluidLoadable & TSharedObject;
}

// (No @packageDocumentation comment for this package)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

// @public @sealed
export interface SharedObjectKind<out TSharedObject = unknown> extends ErasedType<readonly ["SharedObjectKind", TSharedObject]> {
is(value: IFluidLoadable): value is IFluidLoadable & TSharedObject;
}

// (No @packageDocumentation comment for this package)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

// @public @sealed
export interface SharedObjectKind<out TSharedObject = unknown> extends ErasedType<readonly ["SharedObjectKind", TSharedObject]> {
is(value: IFluidLoadable): value is IFluidLoadable & TSharedObject;
}

// (No @packageDocumentation comment for this package)
Expand Down
29 changes: 23 additions & 6 deletions packages/dds/shared-object-base/src/sharedObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import { EventEmitterEventType } from "@fluid-internal/client-utils";
import { AttachState } from "@fluidframework/container-definitions";
import type { IDeltaManager } from "@fluidframework/container-definitions/internal";
import { ITelemetryBaseProperties, type ErasedType } from "@fluidframework/core-interfaces";
import { type IFluidHandleInternal } from "@fluidframework/core-interfaces/internal";
import {
type IFluidHandleInternal,
type IFluidLoadable,
} from "@fluidframework/core-interfaces/internal";
import { assert } from "@fluidframework/core-utils/internal";
import {
IChannelServices,
IChannelStorageService,
} from "@fluidframework/datastore-definitions/internal";
import {
type IChannel,
IChannelAttributes,
type IChannelFactory,
IFluidDataStoreRuntime,
Expand Down Expand Up @@ -868,9 +870,14 @@ export interface ISharedObjectKind<TSharedObject> {
* @sealed
* @public
*/
// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface SharedObjectKind<out TSharedObject = unknown>
extends ErasedType<readonly ["SharedObjectKind", TSharedObject]> {}
extends ErasedType<readonly ["SharedObjectKind", TSharedObject]> {
/**
* Check whether an {@link @fluidframework/core-interfaces#IFluidLoadable} is an instance of this shared object kind.
* @remarks This should be used in place of `instanceof` checks for shared objects, as their actual classes are not exported in Fluid's public API.
*/
is(value: IFluidLoadable): value is IFluidLoadable & TSharedObject;
}

/**
* Utility for creating ISharedObjectKind instances.
Expand All @@ -884,15 +891,25 @@ export interface SharedObjectKind<out TSharedObject = unknown>
export function createSharedObjectKind<TSharedObject>(
factory: (new () => IChannelFactory<TSharedObject>) & { readonly Type: string },
): ISharedObjectKind<TSharedObject> & SharedObjectKind<TSharedObject> {
const result: ISharedObjectKind<TSharedObject> = {
const result: ISharedObjectKind<TSharedObject> &
Omit<SharedObjectKind<TSharedObject>, "brand"> = {
getFactory(): IChannelFactory<TSharedObject> {
return new factory();
},

create(runtime: IFluidDataStoreRuntime, id?: string): TSharedObject {
return runtime.createChannel(id, factory.Type) as TSharedObject;
},

is(value: IFluidLoadable): value is IFluidLoadable & TSharedObject {
return isChannel(value) && value.attributes.type === factory.Type;
},
};

return result as typeof result & SharedObjectKind<TSharedObject>;
}

function isChannel(loadable: IFluidLoadable): loadable is IChannel {
// This assumes no other IFluidLoadable has an `attributes` field, and thus may not be fully robust.
return (loadable as IChannel).attributes !== undefined;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "assert";

import type { IFluidLoadable } from "@fluidframework/core-interfaces";
import type {
IChannel,
IChannelAttributes,
IChannelFactory,
IChannelServices,
IFluidDataStoreRuntime,
} from "@fluidframework/datastore-definitions/internal";
import { MockFluidDataStoreRuntime } from "@fluidframework/test-runtime-utils/internal";

import { createSharedObjectKind } from "../sharedObject.js";

interface IFoo {
foo: string;
}
class SharedFooFactory implements IChannelFactory<IFoo> {
public static readonly Type: string = "SharedFoo";
public readonly type: string = SharedFooFactory.Type;
public readonly attributes: IChannelAttributes = {
type: SharedFooFactory.Type,
snapshotFormatVersion: "0.1",
};
async load(
runtime: IFluidDataStoreRuntime,
id: string,
services: IChannelServices,
channelAttributes: Readonly<IChannelAttributes>,
): Promise<IFoo & IChannel> {
throw new Error("Method not implemented.");
}
create(runtime: IFluidDataStoreRuntime, id: string): IFoo & IChannel {
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
return {
foo: "bar",
attributes: this.attributes,
id,
// Note: other IChannel methods aren't relevant
} as IFoo & IChannel;
}
}

const SharedFoo = createSharedObjectKind<IFoo>(SharedFooFactory);

describe("createSharedObjectKind's return type", () => {
it("delegates to runtime.createChannel on creation", () => {
const createChannelCalls: [id: string | undefined, type: string][] = [];
const runtime = new MockFluidDataStoreRuntime();
runtime.createChannel = (id: string | undefined, type: string) => {
createChannelCalls.push([id, type]);
return null as unknown as IChannel;
};
SharedFoo.create(runtime);
assert.deepEqual(createChannelCalls, [[undefined, SharedFooFactory.Type]]);
createChannelCalls.length = 0;
SharedFoo.create(runtime, "test-id");
assert.deepEqual(createChannelCalls, [["test-id", SharedFooFactory.Type]]);
});

describe(".is", () => {
it("returns true for objects created by the factory", () => {
const factory = SharedFoo.getFactory();
const foo = factory.create(new MockFluidDataStoreRuntime(), "test-id");
assert(SharedFoo.is(foo));
});
describe("returns false for", () => {
const cases: [name: string, obj: unknown][] = [
["object without attributres", {}],
["object with wrong type", { attributes: { type: "NotSharedFoo" } }],
];
for (const [name, obj] of cases) {
it(name, () => {
assert(!SharedFoo.is(obj as IFluidLoadable));
});
}
});
});
});
6 changes: 5 additions & 1 deletion packages/dds/shared-summary-block/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_SharedSummaryBlock": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ declare type current_as_old_for_InterfaceDeclaration_ISharedSummaryBlock = requi
* typeValidation.broken:
* "VariableDeclaration_SharedSummaryBlock": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_SharedSummaryBlock = requireAssignableTo<TypeOnly<typeof old.SharedSummaryBlock>, TypeOnly<typeof current.SharedSummaryBlock>>

/*
Expand Down
6 changes: 5 additions & 1 deletion packages/dds/task-manager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@
"typescript": "~5.4.5"
},
"typeValidation": {
"broken": {}
"broken": {
"VariableDeclaration_TaskManager": {
"forwardCompat": false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ declare type current_as_old_for_TypeAliasDeclaration_TaskEventListener = require
* typeValidation.broken:
* "VariableDeclaration_TaskManager": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
declare type old_as_current_for_VariableDeclaration_TaskManager = requireAssignableTo<TypeOnly<typeof old.TaskManager>, TypeOnly<typeof current.TaskManager>>

/*
Expand Down
Loading

0 comments on commit 6bdec1a

Please sign in to comment.