Migrate four more shared objects to use SharedObjectKind #20985
Conversation
github-actions
bot
added
area: dds
| @@ -14,6 +14,9 @@ import { IJSONRunSegment } from '@fluidframework/sequence/internal'; | |||
| import { IJSONSegment } from '@fluidframework/merge-tree/internal'; | |||
| import { ISegment } from '@fluidframework/merge-tree/internal'; | |||
| import { ISharedObject } from '@fluidframework/shared-object-base'; | |||
| import { ISharedObjectEvents } from '@fluidframework/shared-object-base'; | |||
| import { ISharedObjectKind } from '@fluidframework/shared-object-base/internal'; | |||
| import { ISharedObjectKind as ISharedObjectKind_2 } from '@fluidframework/shared-object-base'; | |||
There was a problem hiding this comment.
What's the reason for both imports, especially with one being internal?
There was a problem hiding this comment.
This is due to microsoft/rushstack#4654 interacting poorly with how we do our release tag scoping. Its possible to avoid it if you ensure all imports of something that are also exported use the same exact import path, across all our packages. I'll see if its easy to work around in this case.
There was a problem hiding this comment.
imports match in are identical in this package, so this is a new case of this problem for me. Fortunately, it just impacts API extractor reports (but maybe also docs? But we don't currently do docs for alpha/internal, so this should not matter). Thus I see no way to fix this, and no significant cost to leaving it, so I think I'll leave it, though it would be nice to understand better.
There was a problem hiding this comment.
Makes sense, it just seemed a bit odd to me but it seems fine to leave in then.
| @@ -14,7 +14,12 @@ import { | |||
| MockStorage, | |||
| } from "@fluidframework/test-runtime-utils/internal"; | |||
|
|
|||
| import { SparseMatrix, SparseMatrixFactory, SparseMatrixItem } from "../sparsematrix.js"; | |||
| import { | |||
| SparseMatrix, | |||
There was a problem hiding this comment.
Is this needed if we now have SparseMatrixClass?
There was a problem hiding this comment.
SparseMatrixClass could be used instead as a type (currently they are the same since sparse matrix does not define an interface like most of our shared object, which is required for it to avoid leaking its class (and thus SharedObject) into the API. Since its only @internal that seems fine.
Anyway, looking at the test usage, migrating this to use SparseMatrixClass type would produce a larger diff for no real gain, but I did see it could use the newer factory pattern to create instance in the test via SparseMatrix.create, so I updated it to use that.
packages/dds/ordered-collection/api-report/ordered-collection.api.md
Outdated
Show resolved
Hide resolved
These are all internal or alpha, and still export the classes in case something actually needs them. This continues the work in changes like #20939 and #20985 moving away from depending the SharedObject subclasses directly as factories and using the common SharedObjectKind type to allow sharing of docs and implementations for the factory APIs. ## Breaking Changes Users of PactMap, SharedSummaryBlock or TaskManager may need to adjust their code if their use did not align with the intended patterns.
Description
These are all internal or alpha, and still export the classes in case something actually needs them.
This continues the work in changes like #20939, moving away from depending the SharedObject subclasses directly as factories and using the common SharedObjectKind type to allow sharing of docs and implementations for the factory APIs.
Breaking Changes
Users of SharedNumberSequence, SharedObjectSequence, SparseMatrix or ConsensusQueue will need to update their code to use the *Class exports if they have a dependency on something static from class that's not on the shared object kind, like its constructor.
Reviewer Guidance
The review process is outlined on this wiki page.