feat(odsp-client): Expose container serialize and rehydrate functionality #25802
Conversation
github-actions
bot
added
area: framework
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
| /** | ||
| * Serialize a detached container to a string representation. This can be saved for later rehydration. | ||
| */ | ||
| serialize(): string; |
There was a problem hiding this comment.
Couple thoughts:
- IMO it's a flaw in the underlying API that we have diverging paths for detached containers (serialize/rehydrate) as compared to attached containers (getPendingLocalState and load props). Probably not reasonable to fix on your timeline, but just to set context.
- The API naming has always been awful too, but at least that's easier to paper over. So I'd suggest renaming at this layer to both:
- More clearly indicate that this is for detached containers only (such that throwing for non-detached containers isn't a surprise)
- More clearly pair it with its partner API, rehydrate.
Brainstorming some options:
- dehydrateDetachedContainer/rehydrateDetachedContainer
- saveDetachedContainer/loadDetachedContainer
- serializeDetachedContainer/loadFromSerialized
There was a problem hiding this comment.
Ah, thank you for the naming context. I'm liking the direction of serializeDetachedContainer/loadFromSerialized. Maybe serializeDetachedContainer/createFromSerializedContainer?
| "Cannot serialize container. Container must be in detached state and not closed.", | ||
| ); | ||
| } | ||
| return this.container.serialize(); |
There was a problem hiding this comment.
Serialization implies a compatibility contract against the outputted serialized form, e.g. if the customer serializes to a string, saves that somewhere, updates to a newer version of Fluid, then tries to use that serialized form to load -- probably needs to work within some support window.
- I don't know if we've discussed what that support window/contract is today for its current legacy/beta tagging on IContainer
- I suspect that since this PR exposes it on a public API, it might increase that window/contract.
We should at least document the lifespan/expectation of compat that we're giving as we expose this API, if not include more programmatic safeguards like timebomb/version checks.
This might merit some discussion, definitely also get @anthony-murphy 's opinion as it also includes the serialized form from getPendingLocalState.
There was a problem hiding this comment.
I believe this PR is exposing it only through ODSP's FluidContainer, so it's Beta, but let me know if I missed something.
I agree the timing window is tricky. For the current usecase, they are basically serializing a "guest" user, logging in, then pretty immediately loading from that serialized state via localStorage.
I would think we could, for now, possibly say "only compatible with current version and tree schema" then improve that compatibility over time, rather than spending a lot of effort right now to meet some arbitrary goal.
| /** | ||
| * Serialize the container to a string representation. This can be saved for later rehydration. | ||
| */ | ||
| serialize(): string; |
There was a problem hiding this comment.
Serializing isn't service specific, so I'd expect to find it on IFluidContainer (even if AzureClient doesn't have a method to rehydrate).
There was a problem hiding this comment.
It is on IFluidContainer (internal version), but this is a way to expose some functionality of IFluidContainer with beta rather than public exposure.
2 participants
Description
Portion of #25597 to dedupe OdspClient functionality from partner codebase.
This PR exposes existing serialize and rehydrate functionality via
OdspClient.rehydrateContainer()andOdspFluidContainer.serialize()Reviewer Guidance
The serialize functionality has a lot of tests in the code currently, but likely need more from an end-to-end standpoint for cross compat, or we can document that the serialized container only works for the current fluid version?