improvement(client): improved internal submitMessage, reSubmit, rollback, and submitSignal type-safety #24319
Conversation
…ack, and submitSignal type-safety
In Legacy API:
- Add type parameter to `IEnvelope` for `contents` to support stricter typing where desired.
- Deprecate `FluidDataStoreRuntime.submitMessage` as not required per `IFluidDataStoreChannel`. `IFluidParentContext` (which is base interface for `IFluidDataStoreContext`) should always be used to access `submitMessage` functionality.
- Remove unused parameters from mock `submitMessage` and `submitSignal` methods.
Internally:
- Replace `any` in `ISignalEnvelope` with `unknown`.
- Relocate `FluidDataStoreMessage` to runtime-definitions. Eventually this can serve as basis for message types for DataStores.
- In `datastore`, create `LocalFluidDataStoreRuntimeMessage` representing the two known messages for DataStores.
- `FluidDataStoreContext` requires `OutboundFluidDataStoreMessage` for stricter `reSubmit` and `rollback` methods.
- Create `IFluidRootParentContext` for the transition from DataStore to `ContainerRuntime` and use explicit types in `ContainerRuntime` and `ChannelCollection`.
- In `ChannelCollection` restore some old method names with strict typing that internal callers must conform to. The `IFluidDataStoreChannel` portions that were not required by current use were refactored to new `ComposableChannelCollection` class.
- Replace `wrapContext` with `formParentContext` that handles both `IFluidParentContext` and `IFluidRootParentContext` by requiring the specific `submitMessage` and `submitSignal` functions
- Within `ContainerRuntime`:
- Update `submitMessage` to expect specific messages
- Update `submitSignal` to expect new `AddressedSignalEnvelope` that is specific subset of `ISignalEnvelope` and is exactly `IEnvelope<ISignalEnvelope["contents"]>`
- Fixup tests to use more realistic data (as required by stricter typing).
Potential Future Improvements:
- Change {@link IFluidDataStoreChannel} and {@link IFluidParentContext}, to have a generic specifying `T extends FluidDataStoreMessage` and uses `T["type"]` and `T["content"]` to qualify message related methods, preferably where `submitMessage`, `reSubmit`, and `rollback` have overloads to ensure callers pair values correctly.
- A further improvement would be to reshape `submitMessage`, `reSubmit`, and `rollback` to accept `T` as `message` parameter instead of `type` and `content` parameters that are hard to convince TypeScript must be paired in implementations.
jason-ha
requested review from
markfields and
vladsud
and removed request for
Copilot
github-actions
bot
added
area: framework
There was a problem hiding this comment.
Copilot reviewed 26 out of 27 changed files in this pull request and generated no comments.
Files not reviewed (1)
- packages/runtime/datastore/package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/framework/attributor/src/runtimeAttributorDataStoreChannel.ts:189
- The reSubmit method in this attributor channel now has no parameters, which deviates from the rest of the interface. Consider adding documentation or a comment clarifying that reSubmit is intentionally not supported in this context.
public reSubmit(): void {
packages/runtime/container-runtime/src/channelCollection.ts:724
- [nitpick] The naming of 'reSubmitContainerMessage' (and the corresponding 'rollbackDataStoreOp') differs from the standard naming conventions used in other parts of the code. Consider standardizing the naming across modules for easier maintainability.
public readonly reSubmitContainerMessage = (
| /** | ||
| * @deprecated No implementation required and will be removed in 2.50. | ||
| */ | ||
| public submitMessage(type: DataStoreMessageType, content: any, localOpMetadata: unknown) { |
There was a problem hiding this comment.
This deprecation is not required. Just seemed like it might be stale and a clean-up opportunity.
There was a problem hiding this comment.
Yeah I've noticed this isn't used too I think. The asymmetry with submitSignal caught my eye, but I guess the point is, op semantics are internal to the FluidDataStore/DDS implementations, and DataStore authors don't get to generate them manually. Signals is an open protocol that the runtime makes available to DataStore authors.
There was a problem hiding this comment.
(hence submitMessage not existing on IFluidDataStoreRuntime)
…jected types for `submitSignal` and `submitMessage`. Additionally, deprecate `MockFluidDataStoreRuntime.submitMessage`.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md
Show resolved
Hide resolved
| * @internal | ||
| * | ||
| * @privateRemarks | ||
| * Future use opportunity: |
There was a problem hiding this comment.
Ok I'm talking out of both sides of my mouth - in the other comment I talked down adding these generics, didn't immediately connect the dots to this comment til now. I totally get the structure here and it's obviously appealing in one sense, but I'm concerned about how it will feel in practice. Maybe too many generics? Other downsides I'm not thinking of?
| * | ||
| * @privateRemarks | ||
| * `type` parameter's type of `DataStoreMessageType` is a covariance exception | ||
| * over `string` that `IFluidDataStoreChannel` specifies. (`unreachableCase` |
There was a problem hiding this comment.
Some thoughts on this - It's strictly true, but only messages that were submitted from here will come back for resubmit. So IMO it is a legitimate, stable and robust way to type this. I don't think the warning about unreachableCase being hit over time is warranted.
I believe it's correct that IFluidDataStoreChannel should specify string here, since another implementation could have its own schema. And we haven't wanted to parametrize everything, it leads to quite a soup of generics and doesn't provide much value in practice IMO.
| * | ||
| * @privateRemarks | ||
| * `type` parameter's type of `DataStoreMessageType` is a covariance exception | ||
| * over `string` that `IFluidDataStoreChannel` specifies. |
There was a problem hiding this comment.
Same as above. If you're going to call this out, please also point out that the only values passed in here at runtime are those passed out via this.dataStoreContext.submitMessage
There was a problem hiding this comment.
I do believe that is the intent, yet the contract (typing) doesn't say that is definitely the case. It is up to the caller to respect that and not have bugs. I am pretty sure there are improvements that can be made to provide safety and help with readability.
But, yes, we can note the intent in comments in the meantime.
| * Type of the op, within the ContainerRuntime's domain | ||
| */ | ||
| type: TType; | ||
| // eslint-disable-next-line @typescript-eslint/no-namespace |
There was a problem hiding this comment.
Talk to me about namespace. What does it do, why is the linter saying don't do it, and why is it so rare in our codebase?
There was a problem hiding this comment.
I can't get answers to why the linter says don't do it. It may stem from when there was a bunch of churn in the space with ESM modules and confusing terminology.
This was more useful earlier when I was exploring things and it looked like this might leak out. Can and probably should just roll this back.
| } | ||
|
|
||
| type SubmitKeys = "submitMessage" | "submitSignal"; |
There was a problem hiding this comment.
Nit: Either export as internal and use in IFluidRootParentContext or inline these in both places.
There was a problem hiding this comment.
Hmm. There is a separation of concerns here that happens to coincide because of this utility and makes it look like they must be the same. In practice there can be other differences between the two interfaces.
I'll want to look at what is going on with IFluidParentContextPrivate. Might be some changes per that.
| export function formParentContext<T extends IFluidParentContext | IFluidRootParentContext>( | ||
| context: Omit<IFluidParentContext & IFluidRootParentContext, SubmitKeys>, | ||
| overrides: Pick<T, SubmitKeys>, | ||
| ): Omit<IFluidParentContext & IFluidRootParentContext, SubmitKeys> & Pick<T, SubmitKeys> { |
There was a problem hiding this comment.
Here's a good time to plug this PR of mine I never pushed through #23694 Review comments welcome even as it's closed, I do expect to reopen it.
There was a problem hiding this comment.
Mutable should probably go next to Deep/ShallowReadonly in core-interfaces
| * @internal | ||
| * @privateRemarks Exposed per ChannelCollection testing and API extractor request | ||
| */ | ||
| export interface IFluidRootParentContext |
There was a problem hiding this comment.
You'll need to rationalize splitting this out with the new IFluidParentContextPrivate added by #24465
There was a problem hiding this comment.
Maybe "reconcile" is what you meant. Yeah, probably having this first (proposed way before) would have been good. This goes a lot further on splitting out the nested datastore support and might simplify or clarify things.
| @@ -708,11 +711,11 @@ export class ContainerRuntime | |||
| extends TypedEventEmitter<IContainerRuntimeEvents> | |||
| implements | |||
| IContainerRuntime, | |||
| Omit<IFluidRootParentContext, "submitSignal">, | |||
There was a problem hiding this comment.
Toss a comment here explaining why the mismatch? (it's a type mismatch that leads you to omit this right?)
There was a problem hiding this comment.
Can do. It is not a type mismatch. ContainerRuntime doesn't need to implement any of IFluidRootParentContext. It does implement most of it so that it can hand itself over to the formParentContext function to make an IFluidRootParentContext. So, the utility here is to force type checking on those members in the class versus there being a type failure when this is given to formParentContext.
| @@ -4461,7 +4464,7 @@ export class ContainerRuntime | |||
| case ContainerMessageType.Alias: { | |||
| // For Operations, call resubmitDataStoreOp which will find the right store | |||
There was a problem hiding this comment.
Out of date / out of context comment?
There was a problem hiding this comment.
Perhaps a little out of date. I suspect this case only handled FluidDataStoreOp to begin with and was expanded for Attach and Alias later.
|
|
||
| Implementing `FluidDataStoreRuntime.submitMessage` is not required per `IFluidDataStoreChannel` and is now deprecated on `FluidDataStoreRuntime` and corresponding `MockFluidDataStoreRuntime`. | ||
|
|
||
| See [issue #24406](https://github.com/microsoft/FluidFramework/issues/24406) for details and alternatives. |
There was a problem hiding this comment.
That issue mentions:
IFluidParentContext (the base interface for IFluidDataStoreContext) should always be used to access submitMessage functionality.
That may be true for inside FluidDataStoreRuntime class, but as for the public API to submit an op, I think the comment should just be - "Submitting arbitrary messages is not supported"
There was a problem hiding this comment.
Is FluidDataStoreRuntime a class customers are expected to derive from? I don't think Pages does and I see that we do internally in at least two places.
Feel free edit the issue.
We should seal the class if no one should derive from it. Then it will be easier to blanketly say don't call.
In Legacy API:
IEnvelopeforcontentsto support stricter typing where desired.submitMessageonFluidDataStoreRuntime andMockFluidDataStoreRuntimeas not required perIFluidDataStoreChannel.IFluidParentContext(which is base interface forIFluidDataStoreContext) should always be used to accesssubmitMessage` functionality.submitMessageandsubmitSignalmethods.Internally:
anyinISignalEnvelopewithunknown.FluidDataStoreMessageto runtime-definitions. Eventually this can serve as basis for message types for DataStores.datastore, createLocalFluidDataStoreRuntimeMessagerepresenting the two known messages for DataStores.FluidDataStoreContextrequiresOutboundFluidDataStoreMessagefor stricterreSubmitandrollbackmethods.IFluidRootParentContextfor the transition from DataStore toContainerRuntimeand use explicit types inContainerRuntimeandChannelCollection.ChannelCollectionrestore some old method names with strict typing that internal callers must conform to. TheIFluidDataStoreChannelportions that were not required by current use were refactored to newComposableChannelCollectionclass.wrapContextwithformParentContextthat handles bothIFluidParentContextandIFluidRootParentContextby requiring the specificsubmitMessageandsubmitSignalfunctionsContainerRuntime:submitMessageto expect specific messagessubmitSignalto expect newAddressedSignalEnvelopethat is specific subset ofISignalEnvelopeand is exactlyIEnvelope<ISignalEnvelope["contents"]>Potential Future Improvements:
T extends FluidDataStoreMessageand usesT["type"]andT["content"]to qualify message related methods, preferably wheresubmitMessage,reSubmit, androllbackhave overloads to ensure callers pair values correctly.submitMessage,reSubmit, androllbackto acceptTasmessageparameter instead oftypeandcontentparameters that are hard to convince TypeScript must be paired in implementations.