Type Erase Shared object kinds #21081

CraigMacomber · 2024-05-14T22:51:45Z

Description

Make ISharedObjectKind, IChannel, IChannelFactory, IFluidDataStoreRuntime and more alpha by tweaking fluid-static APIs typing.

Breaking Changes

Access to these now less public types should not be required for users of the @public "declarative API" exposed in the fluid-framework package, but can still be accessed for those who need them under the /legacy import paths.
The full list of such types is:

SharedTree as exported from @fluidframwork/tree: It is still exported as @public from fluid-framework as SharedObjectKind.
ISharedObjectKind: See new SharedObjectKind type for use in @public APIs.
ISharedObject
IChannel
IChannelAttributes
IChannelFactory
IExperimentalIncrementalSummaryContext
IGarbageCollectionData
ISummaryStats
ISummaryTreeWithStats
ITelemetryContext
IDeltaManagerErased
IFluidDataStoreRuntimeEvents
IFluidHandleContext
IProvideFluidHandleContext

Removed APIs:

DataObjectClass: Usages replaced with SharedObjectKind.
LoadableObjectClass: Replaced with SharedObjectKind.
LoadableObjectClassRecord: Replaced with Record<string, SharedObjectKind>.

Reviewer Guidance

The review process is outlined on this wiki page.

CraigMacomber · 2024-05-14T23:20:28Z

packages/dds/shared-object-base/src/sharedObject.ts

+ * @public
+ */
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+export interface SharedObjectKind<out TSharedObject = unknown>


This is the important new type in this change. The old ISharedObjectKind is appropriate for the encapsulated API, and this is a type erased version removing its members for use in the declarative API.

We try to start all interfaces with "I".
I'm not sure that dropping I makes it clearer on what type actually means / where to use it.

Should there be any limitations on TSharedObject? Like extend ISharedObject?

We try to start all interfaces with "I".
I'm not sure that dropping I makes it clearer on what type actually means / where to use it.

Generally, we have been naming types with members that define some sort of contract, usually a collection of methods "I" something.
This type is not like that. It has no members and is not implementable. Its an opaque token as far as the user of it is concerned.
It is defined using interface because TSC produces better IntelliSense and error messages if we use interface then if we use type in this case (using interface introduces a name that can occur in messages while type does not).
As an implementation detail this type actually extends the ErasedType class (as thats the only want to make a non-implementable type), so its not even a real interface from the class vs interface perspective.
Thus I think not having an "I" is acceptable here.

The version with the "I" is an actual interface which is alpha/legacy, actually legal and safe to implement, and has the API surface needed for the encapsulated API. Only SharedObjectKind is public, so public users shouldn't get confused.

Encapsulated API users should stick with the one that actually has members they can access, but getting it wrong isn't very risky as the compiler will let you know and the runtime behavior would work in most cases anyway.

That was my reasoning, but if you have other name suggestions, I'd be happy to consider them. This was just the first option I thought of, and seemed ok and didn't require renaming existing types.

Should there be any limitations on TSharedObject? Like extend ISharedObject?

I think not, but thats not a strong opinion.

I don't think adding such an extends clause gains us anything (other than perhaps some documentation, which might be worth it). Its mainly helpful if for code works with SharedObjectKinds without using strong typing of its output, which we don't do (since we down cast to internal types first), and if someone did want such code, they can just include the constraint it in their own code. Generally I find it cleaner to leave the constrains out unless we have a reason to include them.

For this specific case if we constrained it to ISharedObject, that would add a bunch of types back to the public API. If we wanted a constraint here IFluidLoadable would be the good choice as thats all ITree extends publicly.
Additionally when we actually stabilize any DataObjects, they can work via this type without us adding any extra API surface (they work correctly with it currently: we just don't have any public non-experimental data objects), so that would also suggest IFluidLoadable if we want a constraint here.

Lastly as this is a type only we implement, adding constraints to is non-breaking, but removing them is breaking, which is another reason to side on leaving it unconstrained for now.

CraigMacomber · 2024-05-14T23:23:19Z

packages/framework/aqueduct/api-report/aqueduct.api.md

@@ -84,6 +85,9 @@ export interface ContainerRuntimeFactoryWithDefaultDataStoreProps {
    runtimeOptions?: IContainerRuntimeOptions;
 }

+// @internal
+export function createDataObjectKind<T extends new (...any: any[]) => DataObject>(factory: T): T & SharedObjectKind<InstanceType<T>>;


This Unified the APi shape/types for data objects in the declarative API, avoiding adding any extra typing to support them.

CraigMacomber · 2024-05-14T23:25:45Z

packages/framework/fluid-framework/src/index.ts

+	type ReadOnlyInfo,
+	type IConnectionDetails,
+	type IDeltaQueueEvents,
+} from "@fluidframework/container-definitions/internal";


due to our API extractor configuration, these types had to be exported. I'm not clear on the exact cause, but with how package boundaries are handled its not surprising.

This would imply that we added something new to the API here that required pulling all those things in. Can we not just remove whatever that was? I really don't think we want to expose the delta manager types if we can avoid it.

fluid-framework: Error: /home/craig/Work/FluidFramework.git/proxy/packages/dds/shared-object-base/src/sharedObject.ts:145:2 - (ae-forgotten-export) The symbol "IDeltaManager" needs to be exported by the entry point index.d.ts

So thats from SharedObjectCore.

fluid-framework: Error: /home/craig/Work/FluidFramework.git/proxy/packages/dds/shared-object-base/src/sharedObject.ts:626:1 - (ae-forgotten-export) The symbol "SharedObjectCore" needs to be exported by the entry point index.d.ts

So thats from SharedObject.

fluid-framework: Error: /home/craig/Work/FluidFramework.git/proxy/packages/dds/sequence/src/sequence.ts:116:1 - (ae-forgotten-export) The symbol "SharedObject" needs to be exported by the entry point index.d.ts

Which we need because of SharedSegmentSequence. Which was already exported.

No change was made to the relevant exports which depend on these things.

What did change is we explicitly export something @fluidframework/shared-object-base, which causes to have to export all direct dependencies of existing exports that are from that package, and all the things they reference in packages we explicitly export from recursively.

While this could be fixed by not exporting anything from @fluidframework/shared-object-base, I don't think thats a good solution as we want to export SharedObjectKind, and moving it to another package to doge this issue would be silly.

I tried to clean this up by requiring all transitive referenced types to be exported in #20554 to avoid this mess and make thing much more clear, but I was unable to get support for that change.

Thus I don't see what I should do other than what I did here, which I agree is bad and confusing. I'm open to suggestions.

ae-forgotten-exports is not currently enabled in this package, right? Or was that enabled at some point?

If any of these types were already transitively referenced, then I'm concerned that they weren't already an issue. If they are newly transitively referenced, then I think we need to find a solution that doesn't leak them. Or we can re-disable the forgotten exports rule until we fixed things here. I would definitely prefer the former, but the latter is probably fine if we follow up quickly.

@jason-ha FYI

Based on the docs in common/build/build-common/api-extractor-lint.json, I think we suppress ae-forgotten-export giving it log level none, but thats only about cross package stuff? The config isn't super clear to me.

packages/framework/fluid-static/package.json

CraigMacomber · 2024-05-14T23:28:58Z

packages/framework/fluid-static/api-report/fluid-static.api.md


 // @public
 export type ContainerAttachProps<T = unknown> = T;

 // @public
 export interface ContainerSchema {
-    readonly dynamicObjectTypes?: readonly LoadableObjectClass[];
-    readonly initialObjects: LoadableObjectClassRecord;
+    readonly dynamicObjectTypes?: readonly SharedObjectKind[];


This is the other half of the main public APi change: using SharedObjectKind instead of LoadableObjectClass.

someone owning this space should take a look. @ChumpChief , @sashasimic, can someone take a look?

LoadableObjectClass currently permits DataObjects, e.g. Signaler, so this change will change the functional scenarios we support. Is that intentional, or are you only intending to change typing while retaining the same scenario support?

I'm personally OK with removing the support as it was only declared "beta", but then you'll need to follow this up with documentation updates to reflect the reduced support (e.g. https://fluidframework.com/docs/build/data-modeling/#creating-a-dynamic-object, probably some tsdoc in various places, probably others).

We still support data objects. No change to what works, they just use an identical public API to shared objects now. I even added a utility to aqueduct to create data objects with the correct typers to with this updated API, and updated data objects tests and examples to align with it.

The public facing docs don't mention this as data objects aren't available publicly in any way that's observable different from shared objects but if we did publish a DataObject as public (for example the experimental one in our tree-react-utils), they could just use it as if it were a shared object and it would just work.

The docs on SharedObjectKind do call out that it can be a DataObject, but only in internal facing docs.

CraigMacomber · 2024-05-14T23:30:27Z

packages/runtime/datastore-definitions/api-report/datastore-definitions.api.md

@@ -89,7 +89,7 @@ export interface IDeltaHandler {
 // @public
 export type IDeltaManagerErased = ErasedType<"@fluidframework/container-definitions.IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>">;

-// @public @sealed
+// @alpha @sealed


Several types moved from public to alpha in here, including IFluidDataStoreRuntime

CraigMacomber · 2024-05-14T23:31:09Z

packages/runtime/runtime-definitions/api-report/runtime-definitions.api.md

@@ -289,7 +289,7 @@ export interface IFluidParentContext extends IProvideFluidHandleContext, Partial
    uploadBlob(blob: ArrayBufferLike, signal?: AbortSignal): Promise<IFluidHandleInternal<ArrayBufferLike>>;
 }

-// @public
+// @alpha


another newly non public API

CraigMacomber · 2024-05-14T23:32:25Z

packages/test/test-version-utils/src/testApi.ts

@@ -43,6 +43,13 @@ import { TestFluidObjectFactory } from "@fluidframework/test-utils/internal";
 // ContainerRuntime and Data Runtime API
 import * as semver from "semver";

+// TypeScript generates incorrect imports in the d.ts file if this is not included.
+import { ISharedObjectKind } from "@fluidframework/shared-object-base/internal";


Apparent TSC bug workaround.
TODO: follow-up on this (report for find open issue)

You have a work item tracking your/our portion of follow-up?

CraigMacomber · 2024-05-14T23:43:12Z

packages/framework/fluid-framework/src/index.ts

+ * @public
+ */
+// eslint-disable-next-line import/export
+export const SharedTree: SharedObjectKind<ITree> = OriginalSharedTree;


Here we reexport SharedTree, but with the alpha types (ISharedObjectKind) removed, just keeping the SharedObjectKind.

Should any of your PR comments make it into code comments?

I have improved the docs comments a bit. Let me know if there is anything you spot thats still missing.

Oops, I missed this file, I'll improve its docs in a moment

Should be better now.

msfluid-bot · 2024-05-14T23:56:10Z

⯅ @fluid-example/bundle-size-tests: +21 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	453.98 KB	453.98 KB	■ No change
azureClient.js	551.22 KB	551.23 KB	⯅ +10 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	257.82 KB	257.82 KB	■ No change
fluidFramework.js	359.58 KB	359.59 KB	⯅ +7 Bytes
loader.js	132.91 KB	132.91 KB	■ No change
map.js	41.45 KB	41.45 KB	■ No change
matrix.js	143.67 KB	143.67 KB	■ No change
odspClient.js	519.75 KB	519.76 KB	⯅ +10 Bytes
odspDriver.js	97.29 KB	97.29 KB	■ No change
odspPrefetchSnapshot.js	42.15 KB	42.15 KB	■ No change
sharedString.js	160.19 KB	160.19 KB	■ No change
sharedTree.js	359.58 KB	359.57 KB	⯆ -6 Bytes
Total Size	3.2 MB	3.2 MB	⯅ +21 Bytes

Baseline commit: 2388680

Generated by 🚫 dangerJS against ceeb1af

experimental/dds/tree/api-report/experimental-tree.api.md

…to SharedObjectKind

packages/service-clients/azure-client/api-report/azure-client.api.md

…to SharedObjectKind

vladsud · 2024-05-15T21:23:11Z

packages/dds/shared-object-base/src/types.ts

@@ -54,7 +54,7 @@ export interface ISharedObjectEvents extends IErrorEvent {

 /**
 * Base interface for shared objects from which other interfaces derive. Implemented by SharedObject
- * @public
+ * @alpha


It's fine with me, but maybe worth doble checking that the message is - we do not support 3rd parties creating DDSs. Is that actually the case?

I believe that is correct. @kashms can you confirm this is still the case?

We do not support users of the declarative API doing creating DDSes. If they were going to implement a custom one, they would need to use alpha APIs (the encapsulated API) anyway since SharedObject (the class) is alpha as are a lots of things they would need to interact with (like the DeltaManager).

Yes, @Josmithr alpha(aka /legacy export) is the right designation here for custom DDSes in FF 2

vladsud · 2024-05-15T21:35:32Z

.changeset/bright-icons-flash.md

+"@fluidframework/fluid-static": minor
+"@fluidframework/runtime-definitions": minor
+"@fluidframework/shared-object-base": minor
+"@fluidframework/tree": minor


Feels like it just be major in most places. Not sure it matters (based on where are with our releases).

Our current workflow doesn't use this in a user facing way, and leaving it at the default (computed by the changeset tool) seems to be the simplest option for our current release process. Its not ideal, but it will work more logically once we are post 2.0 (and then the main branch defaulting to minor will be correct data we don't use instead of ambiguous data we don't use).

vladsud · 2024-05-15T21:49:37Z

packages/dds/shared-object-base/src/sharedObject.ts

+ * @public
+ */
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+export interface SharedObjectKind<out TSharedObject = unknown>


I'd recommend creating some readme.md that talks more in detail these types and intended use, and what problems they solve. Just glancing at this type (or comments above), it's not very clear on what is intended usage. Maybe we should rely on developers glancing at current usage in repo to imply these answers, but it feels like having a bit more English would be useful.

Good point.

I have extended the doc comments here quite a bit. I picked putting the text there as its easier to find/link from the code and the readme is more public/use facing and this seems like developer facing info mostly.

If you have anything further you think I should add, let me know.

vladsud · 2024-05-15T21:59:20Z

packages/dds/counter/api-report/counter.api.md

@@ -21,7 +22,7 @@ export interface ISharedCounterEvents extends ISharedObjectEvents {
 }

 // @alpha
-export const SharedCounter: ISharedObjectKind<ISharedCounter>;
+export const SharedCounter: ISharedObjectKind<ISharedCounter> & SharedObjectKind<ISharedCounter>;


Is it true that every place where we use ISharedObjectKind we will overlap with SharedObjectKind? If yes, should we reconsider how ISharedObjectKind is defined?

If it were practical to do so yes. Sadly I don't think there is a good way to do it.

When dealing with type erasure and unimplementable interfaces, this gets a bit nasty.

If we make ISharedObjectKind extend SharedObjectKind, then its impossible to get typescript to type check an implementation of ISharedObjectKind since SharedObjectKind goes out of its way to opt out of structural typing and not be implementable (which it has to, or all values would be usable as ISharedObjectKinds).

If someone is using the encapsulated API, them manually implementing ISharedObjectKind is probably the main reason they would use the type, and it works fine with how the code is now. You only hit this problem if you write code which needs to work with both the encapsulated API, and also the declarative API. Currently I expect only our repo to do that, and it deduplicates all the type nastiness in one function (createSharedObjectKind): this type is implicitly inferred in the implementation as the return value from that function.

I have updated some of the relevtn doc comments to help clarify this.

vladsud · 2024-05-15T22:02:12Z

packages/framework/fluid-framework/src/index.ts

+	type ReadOnlyInfo,
+	type IConnectionDetails,
+	type IDeltaQueueEvents,
+} from "@fluidframework/container-definitions/internal";


Sorry if it's stupid question (have not looked at fluid-framework), but does this reexport essentially makes APIs "public"? I hope the answer is no, but worth checking.

re-exports have no impact on the tagging. These types, which were transitively reachable from existing legacy/alpha API are simply being added to the legacy exports here, but they were already available under legacy on other packages. They were added because API extractor is configured to force it with the current rather indecisive config which forces including some transitive deps but not others (and unrelated changes in this PR impacted which transitive's it requires by impacting which types and packages are referenced directly)

I think its a topic of some debate current if fluid-framework/legacy counts as "public" or more public than legacy on other packages which is why I was all wordy and specific there.

…to SharedObjectKind

Josmithr

Approving for the changes to devtools.

jason-ha

Love it!

jason-ha · 2024-05-17T16:23:42Z

packages/tools/devtools/devtools/src/index.ts

@@ -186,7 +186,7 @@ function mapContainerProps(
 	return {
 		container: innerContainer,
 		containerKey,
-		containerData: container.initialObjects,
+		containerData: container.initialObjects as Record<string, IFluidLoadable>,


I've seen as get us into trouble plenty. Is there nothing better here? Some debt we should follow up on?

jason-ha · 2024-05-17T16:26:01Z

packages/test/test-version-utils/src/testApi.ts

@@ -43,6 +43,13 @@ import { TestFluidObjectFactory } from "@fluidframework/test-utils/internal";
 // ContainerRuntime and Data Runtime API
 import * as semver from "semver";

+// TypeScript generates incorrect imports in the d.ts file if this is not included.
+import { ISharedObjectKind } from "@fluidframework/shared-object-base/internal";


You have a work item tracking your/our portion of follow-up?

## Description Make ISharedObjectKind, IChannel, IChannelFactory, IFluidDataStoreRuntime and more alpha by tweaking fluid-static APIs typing. ## Breaking Changes Access to these now less public types should not be required for users of the `@public` "declarative API" exposed in the `fluid-framework` package, but can still be accessed for those who need them under the `/legacy` import paths. The full list of such types is: - `SharedTree` as exported from `@fluidframwork/tree`: It is still exported as `@public` from `fluid-framework` as `SharedObjectKind`. - `ISharedObjectKind`: See new `SharedObjectKind` type for use in `@public` APIs. `ISharedObject` - `IChannel` - `IChannelAttributes` - `IChannelFactory` - `IExperimentalIncrementalSummaryContext` - `IGarbageCollectionData` - `ISummaryStats` - `ISummaryTreeWithStats` - `ITelemetryContext` - `IDeltaManagerErased` - `IFluidDataStoreRuntimeEvents` - `IFluidHandleContext` - `IProvideFluidHandleContext` Removed APIs: - `DataObjectClass`: Usages replaced with `SharedObjectKind`. - `LoadableObjectClass`: Replaced with `SharedObjectKind`. - `LoadableObjectClassRecord`: Replaced with `Record<string, SharedObjectKind>`.

CraigMacomber added 2 commits May 14, 2024 13:48

Erase SharedObjectKind

3ec4869

Make some alpha. Fix most issues

e6c24e3

Work around TSC bug

f213827

CraigMacomber commented May 14, 2024

View reviewed changes

packages/framework/fluid-static/package.json Outdated Show resolved Hide resolved

CraigMacomber commented May 14, 2024

View reviewed changes

Josmithr reviewed May 15, 2024

View reviewed changes

experimental/dds/tree/api-report/experimental-tree.api.md Outdated Show resolved Hide resolved

Josmithr requested a review from jason-ha May 15, 2024 17:04

CraigMacomber added 3 commits May 15, 2024 10:15

Make more types alpha

a0b3a6d

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

f334597

…to SharedObjectKind

fix type tests

355c414

Merge InternalDataObjectClass and DataObjectClass

bcd1f09

CraigMacomber commented May 15, 2024

View reviewed changes

packages/service-clients/azure-client/api-report/azure-client.api.md Outdated Show resolved Hide resolved

CraigMacomber added 3 commits May 15, 2024 12:22

Less public, add changeset

f415d8d

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

175b6dd

…to SharedObjectKind

Clean build

5382021

CraigMacomber marked this pull request as ready for review May 15, 2024 19:40

CraigMacomber requested review from a team as code owners May 15, 2024 19:40

vladsud reviewed May 15, 2024

View reviewed changes

vladsud approved these changes May 15, 2024

View reviewed changes

CraigMacomber added 3 commits May 15, 2024 15:48

Better doc comments and add readonly

d086750

Improve export doc

b50fe2c

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

45ae305

…to SharedObjectKind

Josmithr approved these changes May 16, 2024

View reviewed changes

alex-pardes approved these changes May 16, 2024

View reviewed changes

Merge branch 'main' into SharedObjectKind

ceeb1af

anthony-murphy approved these changes May 17, 2024

View reviewed changes

CraigMacomber merged commit 78f228e into microsoft:main May 17, 2024
30 checks passed

CraigMacomber deleted the SharedObjectKind branch May 17, 2024 16:28

jason-ha reviewed May 17, 2024

View reviewed changes

Type Erase Shared object kinds #21081

Type Erase Shared object kinds #21081

Conversation

CraigMacomber commented May 14, 2024 • edited Loading

Description

Breaking Changes

Reviewer Guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CraigMacomber May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot commented May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmithr left a comment

Choose a reason for hiding this comment

jason-ha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CraigMacomber commented May 14, 2024 •

edited

Loading

CraigMacomber May 14, 2024 •

edited

Loading

msfluid-bot commented May 14, 2024 •

edited

Loading