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 = (
alexvy86
left a comment
alexvy86
left a comment
There was a problem hiding this comment.
Haven't looked at the actual change in detail, but a few doc comments in the meantime
…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}
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md
Outdated
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?
| readonly submitSignal: (envelope: AddressedSignalEnvelope, targetClientId?: string) => void; | ||
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
IFluidParentContextPrivate doesn't appear to be doing much. It is just making two properties required by respecifying them (currently).
Sticking with this separation for now.
| 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
|
Now all caught up and reconciled with main. |
| setConnectionState(connected: boolean, clientId?: string): void; | ||
| // (undocumented) | ||
| submitSignal(type: string, content: any): null; | ||
| submitSignal: IFluidDataStoreRuntime["submitSignal"]; |
There was a problem hiding this comment.
Nit: This is unfortunate. Makes it harder to reason about signature changes that could be made in the future. How critical is it that we explicitly annotate the typing like this? Can we verify compatibility another way such that the signature here remains explicit?
There was a problem hiding this comment.
Type-tests should catch breaking changes made here, so I don't think this is a blocking issue or anything. Would just be nice.
There was a problem hiding this comment.
Is it unfortunate?
The signature for IFluidDataStoreRuntime.submitSignal was changes to (type: string, content: unknown, targetClientId?: string) => void 19 months ago. There was no visit here. This will keep its type up to date.
There was a problem hiding this comment.
It's unfortunate because if IFluidDataStoreRuntime.submitSignal changes again (potentially in a breaking way), there will be no change to the report here, and therefore no indication that a breaking change affected this package (aside from type-test changes for the cases that they detect).
Ideally, API-Extractor could inline things in a case like this, and we could have the best of both worlds, but I'm not holding my breath for that functionality.
I'm not sure what the best answer here is. Presumably we could write a type-test to enforce (at build time) that the signature here matches the signature of IFluidDataStoreRuntime.submitSignal, while keeping the explicit signature. But I don't know if that would be worth the effort.
Again, not blocking on this, just wondering what the best pattern is to ensure we can easily detect changes here.
There was a problem hiding this comment.
Gotcha. Note that there was no reported change before for at least some cases of breaking change to IFluidDataStoreRuntime.submitSignal.
I think the IFluidDataStoreRuntime.submitSignal change should be the larger signal to anything in this space.
packages/test/local-server-tests/src/test/opsOnReconnect.spec.ts
Outdated
Show resolved
Hide resolved
Josmithr
left a comment
Josmithr
left a comment
There was a problem hiding this comment.
API changes look good, assuming we're in the window of time where we can make beta breaks.
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
I think you are referring to The generic addition to If you are concerned about another one of the changes, LMK. |
| /** | ||
| * Submits the signal to be sent to other clients. | ||
| * @param envelope - {@link IEnvelope} containing the signal address and contents. | ||
| * @param targetClientId - When specified, the signal is only sent to the provided client id. |
There was a problem hiding this comment.
Nit
| * @param targetClientId - When specified, the signal is only sent to the provided client id. | |
| * @param targetClientId - When specified, the signal is only sent to the provided client ID. |
There was a problem hiding this comment.
(All I did was copy-paste) I will get it in a follow-up.
…ack, and submitSignal type-safety (microsoft#24319) improvement(client): improved internal submitMessage, reSubmit, rollback, and submitSignal type-safety In Legacy API: - Add type parameter to `IEnvelope` for `contents` to support stricter typing where desired. - Remove unused parameters from mock `submitMessage` and `submitSignal` methods. - Clean up some mock types. 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 `IFluidRootParentContextPrivate` 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 `IFluidParentContextPrivate` and `IFluidRootParentContextPrivate` by requiring the specific `submitMessage` and `submitSignal` functions - Within `ContainerRuntime`: - Update `submitMessage` to expect specific messages - Update `submitSignal` to expect new `AddressedUnsequencedSignalEnvelope` 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.
improvement(client): improved internal submitMessage, reSubmit, rollback, and submitSignal type-safety
In Legacy API:
IEnvelopeforcontentsto support stricter typing where desired.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.IFluidRootParentContextPrivatefor 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 bothIFluidParentContextPrivateandIFluidRootParentContextPrivateby requiring the specificsubmitMessageandsubmitSignalfunctionsContainerRuntime:submitMessageto expect specific messagessubmitSignalto expect newAddressedUnsequencedSignalEnvelopethat 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.