-
Notifications
You must be signed in to change notification settings - Fork 535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate SharedNumberSequence and SharedObjectSequence, and remove most usage of SharedObjectSequence #8585
Deprecate SharedNumberSequence and SharedObjectSequence, and remove most usage of SharedObjectSequence #8585
Conversation
@@ -7,14 +7,12 @@ import { DataObject } from "@fluidframework/aqueduct"; | |||
import { SharedCell } from "@fluidframework/cell"; | |||
import { IFluidHandle } from "@fluidframework/core-interfaces"; | |||
import { SharedMap } from "@fluidframework/map"; | |||
import { SharedObjectSequence } from "@fluidframework/sequence"; | |||
import { IBadgeModel, IBadgeHistory, IBadgeType } from "./Badge.types"; | |||
import { IBadgeModel, IBadgeType } from "./Badge.types"; | |||
import { defaultItems } from "./helpers"; | |||
|
|||
export class Badge extends DataObject implements IBadgeModel { | |||
private _currentCell: SharedCell<IBadgeType> | undefined; | |||
private _optionsMap: SharedMap | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder it this could be refactored to use maxtrix, and just have a single column and insert rows.
this would keep the functionally @tylerbutler wants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd be fine to just remove this part of the change, and open an issue to move it to matrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this functionality as existing for the purpose of demonstrating SharedObjectSequence
as a DDS to use, rather than the functionality being critical to preserve through alternate means. Since we don't recommend SharedObjectSequence
as a DDS to use anymore, I don't think it's particularly troubling to drop the functionality.
For implementation (SharedObjectSequence
vs. matrix
) I think it's not a perfect option either way, since the "source of truth" for latest state is duplicated between the cell and the history. Without grouping the cell set and the history append, I think there's technically risk that the cell and latest history could disagree for simultaneous changes from different clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tylerbutler would be the best person to answer. he brought this up as a use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the history feature from the example is OK with me. We have captured the use-case, and I'm fine with a follow-up to re-add the feature if we conclude it's a good example for how we think matrix (or whatever we decide) should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff.
// Store the handle to the component in the sequence | ||
this.todoItems.insert(this.todoItems.getLength(), [component.handle]); | ||
// Generate a key that we can sort on later, and store the handle. | ||
this.todoItemsMap.set(`${Date.now()}-${uuid()}`, component.handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should put the timestamp in the item, so {handle, timestamp}. it doesn't matter much is this case, but if you wanted to reorder this would be the way, as changes to the sort key, timestamp in this case, would coalesce. Even better than a single object in a single map key would be a subdirectory per entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just don't want the sort by map key in our example, as it prevents move, which is a common next step for a scenario like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly, we could revert this, and open an issue to correctly fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in latest to store the index in the map value rather than sorting on the key.
I am eager to eliminate usage of SharedObjectSequence
to prevent copycatting so don't want to revert, but acknowledge that there will be better solutions. I think we should let this scenario help drive design ideas in #8526.
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output
|
⯅ @fluid-example/bundle-size-tests: +2.17 KB
Baseline commit: 497a706 |
Thanks! :-D |
This change is part of #8526
This change marks
SharedNumberSequence
andSharedObjectSequence
as deprecated to discourage new usage, and removes mostSharedObjectSequence
usage in our repository (replay-tool is left unchanged). It also removes theuseSyncedArray
from@fluid-experimental/react
which depended onSharedObjectSequence
.Changes to examples:
SharedObjectSequence
, leaving it as just "likes"I recognize that some of these example changes are not ideal, but I think we should continue discussion of the scenario in #8526.