Add ISharedSegmentSequence #21067

CraigMacomber · 2024-05-13T23:04:53Z

Description

Fix that ISharedString extends SharedObject (the class).

This resolves one issue preventing tagging of "legacy" DDSes as public since before this change doing so for SharedString would pull in a lot more than it should.

Breaking Changes

ISharedString no longer extends SharedSegmentSequence and instead extends the new ISharedSegmentSequence, which may be missing some APIs.

Attempt to migrate off the missing APIs, but if that is not practical, request the be added to ISharedSegmentSequence and cast to SharedSegmentSequence as a workaround temporally.

Reviewer Guidance

The review process is outlined on this wiki page.

…to ISharedSegmentSequence

CraigMacomber · 2024-05-16T18:26:08Z

examples/data-objects/codemirror/src/codeMirrorView.tsx

@@ -46,7 +46,7 @@ class CodeMirrorView {
 		// https://stackoverflow.com/questions/18828658/how-to-kill-a-codemirror-instance

 		if (this.sequenceDeltaCb) {
-			this.text.removeListener("sequenceDelta", this.sequenceDeltaCb);
+			this.text.off("sequenceDelta", this.sequenceDeltaCb);


This used to leak the underlying event implementation, which had more methods. removeListener is an alias for off, which is the only option now that this is going through the interfaces not exposing the underlying class.

(According to the Node.js docs off is an alias for remvoeListener, but, yeah, we promote use of off; so, cool.)

CraigMacomber · 2024-05-16T18:27:07Z

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

@@ -370,6 +371,31 @@ export interface ISharedIntervalCollection<TInterval extends ISerializableInterv
    getIntervalCollection(label: string): IIntervalCollection<TInterval>;
 }

+// @alpha (undocumented)
+export interface ISharedSegmentSequence<T extends ISegment> extends ISharedObject<ISharedSegmentSequenceEvents>, ISharedIntervalCollection<SequenceInterval>, MergeTreeRevertibleDriver {


I added all the members currently required by code in this repo. There are some more public methods that might belong here, and some that I had to add which should probably be removed.

Most functions that exist here are good (i.e. we encourage/expect real use cases for them for our customers).

getCurrentSeq is probably not something we want to expose, but it's not the end of the world and I don't think we should attempt to fix that here

groupOperation is as you said. would be nice to remove our usage so we can remove the api. but again, not here.

initializeLocal is something we technically did expose but shouldn't be used by customers, and isn't a SharedSegmentSequence thing (so if we wanted to put it somewhere because we were concerned about breaking customers, the right place would be ISharedObject not here, but I think it's probably fine to just remove)

We definitely also want the following:

annotateRange

removeRange

seems like we might have a test gap in terms of using our public API (we definitely have tests that exercise those functions, but I'm guessing they end up typed to the concrete class so you didn't notice)

This leaves a few APIs:

getRangeExtentsOfPosition: Seems like there might be some not very extensive usage of this today based on a search, but it's not really something we need to implement. it can be implemented with other methods on the public API. i'd be ok with either decision.

insertFromSpec: don't think anyone uses this publicly. seems good to remove.

resolveRemoteClientPosition: This looks like it's used today to support some presence scenarios using signals. we probably want it.

posFromRelativePos: If we're including the relative pos methods on SharedString, it seems like we should probably have this as well. That said, i'd love if we could remove all of them as I don't think support is good and not sure anyone's using.

tl;dr is that I think you probably can't get away with removing most of the previously public things on the SharedSegmentSequence class, and I think it might be a bad idea to try to do so as part of a change aimed at hiding the class (it's probably better to do that sort of thing in follow-ups with more targeted guidance on API surface reduction)

@anthony-murphy might have some thoughts here.

Thanks for doing this though in any case. glad to see progress here.

A lot of tests fail to build without initializeLocal here, so if we want to remove it, thats significant work probably best done separately.

There is also getIntervalCollection. As its mentioned in the docs for getIntervalCollectionLabels, I've added that along with the ones you mentioned. Thats all the public members from SharedSegmentSequence. I put some in a / #region APIs we might want to remove to help track them for later.

CraigMacomber · 2024-05-16T18:27:39Z

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

+    // (undocumented)
+    getPropertiesAtPosition(pos: number): PropertySet | undefined;
+    // @deprecated (undocumented)
+    groupOperation(groupOp: IMergeTreeGroupMsg): void;


We depend on this in an example. It should probably be migrated off of it so we can remove this deprecated APi from the new public APi surface.

CraigMacomber · 2024-05-16T18:28:36Z

packages/dds/sequence/src/sequence.ts

+	 * TODO: determine if this API (from SharedObject) is needed by users of the encapsulated API, declarative API or both,
+	 * and handle exposing it in a consistent way for all SharedObjects if needed.
+	 */
+	initializeLocal(): void;


Having this here seems odd, but its currently required.

CraigMacomber · 2024-05-16T18:29:37Z

packages/dds/sequence/src/sharedString.ts

 import { SharedStringFactory } from "./sequenceFactory.js";

 /**
 * Fluid object interface describing access methods on a SharedString
 * @alpha
 */
-export interface ISharedString extends SharedSegmentSequence<SharedStringSegment> {


This is what's getting fixed in this change. Having an interface expend a concreate class leaks the class types into our interface types, defeating the separation the interfaces are supposed to provide.

CraigMacomber · 2024-05-16T18:31:11Z

packages/test/test-end-to-end-tests/src/test/orderSequentially.spec.ts

 		sharedDir = await dataObject.getSharedObject<SharedDirectory>(dirId);
 		sharedCell = await dataObject.getSharedObject<ISharedCell>(cellId);
 		sharedMap = await dataObject.getSharedObject<ISharedMap>(mapId);

 		containerRuntime = dataObject.context.containerRuntime as ContainerRuntime;
 		changedEventData = [];
-		sharedString.on("sequenceDelta", (changed, _local, _target) => {
-			changedEventData.push(changed);
+		sharedString.on("sequenceDelta", (event, _target) => {


This code was wrong and should not have built, but did compile since our event emitting API (on the class) is not very type safe. The interface API is better, and thus forced this to change.

Also an example of why should always test against the "public" API when possible and why class exposure is not preferred.

…mber/FluidFramework into ISharedSegmentSequence

packages/dds/sequence/src/sequence.ts

jason-ha · 2024-05-16T19:04:59Z

examples/data-objects/codemirror/src/codeMirrorView.tsx

@@ -46,7 +46,7 @@ class CodeMirrorView {
 		// https://stackoverflow.com/questions/18828658/how-to-kill-a-codemirror-instance

 		if (this.sequenceDeltaCb) {
-			this.text.removeListener("sequenceDelta", this.sequenceDeltaCb);
+			this.text.off("sequenceDelta", this.sequenceDeltaCb);


(According to the Node.js docs off is an alias for remvoeListener, but, yeah, we promote use of off; so, cool.)

.changeset/loud-maps-fall.md

Abe27342 · 2024-05-16T19:08:55Z

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

+    getPropertiesAtPosition(pos: number): PropertySet | undefined;
+    // @deprecated (undocumented)
+    groupOperation(groupOp: IMergeTreeGroupMsg): void;
+    initializeLocal(): void;


this is defined on SharedObject, not SharedSegmentSequence. I'd rather not export it here. It's unlikely it's used much in consumer code (and if so, breaking change guidance seems fine)

Removing it breaks lots of tests, so I think thats a separate fix.

Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

…mber/FluidFramework into ISharedSegmentSequence

…to ISharedSegmentSequence

## Description Fix that ISharedString extends SharedObject (the class). This resolves one issue preventing tagging of "legacy" DDSes as public since before this change doing so for SharedString would pull in a lot more than it should. ## Breaking Changes ISharedString no longer extends SharedSegmentSequence and instead extends the new ISharedSegmentSequence, which may be missing some APIs. Attempt to migrate off the missing APIs, but if that is not practical, request the be added to ISharedSegmentSequence and cast to SharedSegmentSequence as a workaround temporally.

Add ISharedSegmentSequence

b6eede9

CraigMacomber requested a review from a team as a code owner May 13, 2024 23:04

CraigMacomber marked this pull request as draft May 13, 2024 23:05

github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring base: main PRs targeted against main branch labels May 13, 2024

CraigMacomber added 2 commits May 16, 2024 10:42

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

fa6b77a

…to ISharedSegmentSequence

Fix build

a8be0ff

github-actions bot added area: examples Changes that focus on our examples area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API labels May 16, 2024

CraigMacomber commented May 16, 2024

View reviewed changes

Merge branch 'main' into ISharedSegmentSequence

e907ba0

CraigMacomber marked this pull request as ready for review May 16, 2024 18:34

CraigMacomber added 2 commits May 16, 2024 11:50

Add changeset

3eaa894

Merge branch 'ISharedSegmentSequence' of https://github.com/CraigMaco…

de0ecc5

…mber/FluidFramework into ISharedSegmentSequence

jason-ha approved these changes May 16, 2024

View reviewed changes

Abe27342 reviewed May 16, 2024

View reviewed changes

.changeset/loud-maps-fall.md Outdated Show resolved Hide resolved

Abe27342 reviewed May 16, 2024

View reviewed changes

CraigMacomber and others added 4 commits May 16, 2024 13:02

Add more APIs

589f291

Update .changeset/loud-maps-fall.md

4282486

Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

Remove FF export of SharedSegmentSequence class

348b445

Merge branch 'ISharedSegmentSequence' of https://github.com/CraigMaco…

8c7d59c

…mber/FluidFramework into ISharedSegmentSequence

Abe27342 approved these changes May 17, 2024

View reviewed changes

CraigMacomber added 2 commits May 17, 2024 10:37

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

7caa4bf

…to ISharedSegmentSequence

Update reports from merge

0180298

CraigMacomber mentioned this pull request May 17, 2024

Normalize imports #21140

Merged

CraigMacomber added 2 commits May 17, 2024 11:14

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

8e5110c

…to ISharedSegmentSequence

Fix report

0a3e383

CraigMacomber enabled auto-merge (squash) May 17, 2024 18:18

CraigMacomber merged commit 47465f4 into microsoft:main May 17, 2024
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ISharedSegmentSequence #21067

Add ISharedSegmentSequence #21067

CraigMacomber commented May 13, 2024 •

edited

CraigMacomber May 16, 2024

jason-ha May 16, 2024

CraigMacomber May 16, 2024

Abe27342 May 16, 2024 •

edited

Abe27342 May 16, 2024

Abe27342 May 16, 2024

CraigMacomber May 16, 2024

CraigMacomber May 16, 2024

CraigMacomber May 16, 2024

CraigMacomber May 16, 2024

CraigMacomber May 16, 2024

jason-ha May 16, 2024

jason-ha May 16, 2024

Abe27342 May 16, 2024

CraigMacomber May 16, 2024

Add ISharedSegmentSequence #21067

Add ISharedSegmentSequence #21067

Conversation

CraigMacomber commented May 13, 2024 • edited

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

Abe27342 May 16, 2024 • edited

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 commented May 13, 2024 •

edited

Abe27342 May 16, 2024 •

edited