Declare ContainerRuntime class as implementing interfaces it already implements#24147
Declare ContainerRuntime class as implementing interfaces it already implements#24147markfields merged 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR makes the ContainerRuntime class explicitly declare the interfaces it already implements, which improves code clarity and navigation.
- Added explicit implementation for IFluidParentContext and IGarbageCollectionRuntime.
- Updated import statements to support the new interfaces.
Comments suppressed due to low confidence (2)
packages/runtime/container-runtime/src/containerRuntime.ts:717
- Verify that all required members of IGarbageCollectionRuntime are correctly implemented in ContainerRuntime to ensure full interface compliance.
IGarbageCollectionRuntime,
packages/runtime/container-runtime/src/containerRuntime.ts:720
- Confirm that ContainerRuntime fully implements all expected members of IFluidParentContext, aligning with its interface definition.
IFluidParentContext,
| IGarbageCollectionRuntime, | ||
| ISummarizerRuntime, | ||
| ISummarizerInternalsProvider, | ||
| IFluidParentContext, |
There was a problem hiding this comment.
@agarwal-navin or @vladsud do you know why this wasn't here before? It's legacy-alpha so it wasn't about over-exposure.
For your reference, the spot where CR is passed as this interface is this line in its constructor:
const parentContext = wrapContext(this);
There was a problem hiding this comment.
IFluidParentContext is ideally for contexts. For example, data store context implements it. ContainerRuntime acts as both the runtime and the context for that layer which makes it cumbersome and it has to implement the properties of IFluidParentContext.
The runtime part (IFluidDataStoreChannel) is handled by ChannelCollection. So, it seems fine that the context part is implemented by the ContainerRuntime directly. I don't know why it doesn't do it already, maybe because the legacy-alpha tagging happened after IFluidParentContext was introduced.
There was a problem hiding this comment.
@vladsud do you know why? Any concerns telling TypeScript about this link?
agarwal-navin
left a comment
agarwal-navin
left a comment
There was a problem hiding this comment.
LGTM. I would check with Vlad if there is any specific reason to not implement IFluidParentContext
|
@vladsud any concerns? |
Description
We've hidden the fact that ContainerRuntime class implements certain interfaces to reduce exposure while ContainerRuntime was not internal. Now that it is, we can declare that it implements these interfaces.
It makes the code clearer, and code navigation works better.