Enable calling serialize while container is in the attaching state#19893
Conversation
…to stash-attaching-serialize
…to stash-attaching-serialize
…to stash-attaching-serialize
…y-murphy/FluidFramework-1 into stash-attaching-serialize
github-actions
Bot
added
area: loader
Co-authored-by: Daniel Madrid <105010181+dannimad@users.noreply.github.com>
| * Extract a snapshot of the container as long as it is in detached state. Calling this on an attached container | ||
| * is an error. | ||
| * This function will capture the state of a container this is not Attached or closed. | ||
| * This is useful in draft-like scenarios. For example, think of an email draft, where a user can make changes over time, |
There was a problem hiding this comment.
Not so sure this comment is that related to serialize, it sounds more like detached to me.
There was a problem hiding this comment.
granted, serialize is only used in detached, attaching scenarios but in the phrase "This is useful" not sure if "this" is detached/attaching state or serialize. I guess it makes sense both ways but it was a bit confusing
There was a problem hiding this comment.
Agreed, I have a comment above re: attach() where I think this sort of explanation would fit better.
There was a problem hiding this comment.
all containers start detached, and all container needs to be attached. serialize is what allows preserving unattached container, for draft like scenarios. i don't see why this wouldn't belong here
| * @internal | ||
| */ | ||
| export interface IPendingContainerState { | ||
| attached: true; |
There was a problem hiding this comment.
Didn't know you can set values at the interface definition. Is it safe?
There was a problem hiding this comment.
its not setting a value. its saying the only valid value is true
There was a problem hiding this comment.
it allows union types like IPendingContainerState | IPendingDetachedContainerState to be discriminated via the attached property, such that typescript know that if it is true, it is IPendingContainerState and if it is false it is IPendingDetachedContainerState
| const appSummary: ISummaryTree = this.runtime.createSummary(); | ||
| const protocolSummary = this.captureProtocolSummary(); | ||
| const combinedSummary = combineAppAndProtocolSummary(appSummary, protocolSummary); | ||
| const attachingData = |
There was a problem hiding this comment.
Are the two potential options for the state of the container at this point attaching or detached? And if it's detached, the attachingData should be undefined?
There was a problem hiding this comment.
yes. two possibilities. only an attaching has already created it base summary, so this is handling that, a detached container has no base summary, so we generate it on each call.
Co-authored-by: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Co-authored-by: Daniel Madrid <105010181+dannimad@users.noreply.github.com>
…y-murphy/FluidFramework-1 into stash-attaching-serialize
ChumpChief
left a comment
ChumpChief
left a comment
There was a problem hiding this comment.
I'm ignoring the code changes, just reviewing the docs here.
Love the additions! Most comments are just language suggestions. I call it out in one of the comments, but being pedantic with RFC 2119 keywords can help clarify correct usage to customers.
| /** | ||
| * Attaches the Container to the Container specified by the given Request. | ||
| * When a container is created in Fluid it starts in what we call a detached state, | ||
| * which means it is detached from server storage and unavailable for other users to open or collaborate with. |
There was a problem hiding this comment.
Here would be a good spot to explain why, e.g. "The detached state can be useful for drafting content in the container before the initial upload."
Co-authored-by: Matt Rakow <ChumpChief@users.noreply.github.com>
⯅ @fluid-example/bundle-size-tests: +470 Bytes
Baseline commit: 6dea020 |
…to stash-attaching-serialize
jzaffiro
left a comment
jzaffiro
left a comment
There was a problem hiding this comment.
Changes make sense to me, you may want another opinion though :)
6 participants
Background
This change is the final change for allowing consumers of fluid to call serialize on a container while a container is in an attaching state, which will include a failed attach where the failure didn't cause the container to close. This will aid in the case of offline, network, or server outage by allowing the application to capture the un-attached state of the container, preserve it locally, and later create a new file from it.
AB#5502
Overview
This change enables calling container.serialize while a container is in the attaching state. It is the culmination of many smaller changes that have gone in to enable this change.