improvement(client): Cleanup presence manager interface #22553
Conversation
github-actions
bot
added
base: main
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: e84ac46 |
| function assertSignalMessageIsValid( | ||
| message: IInboundSignalMessage | IExtensionMessage, | ||
| ): asserts message is IExtensionMessage { | ||
| assert(message.clientId !== null, "Signal must have a client ID"); |
There was a problem hiding this comment.
Is this source or target clientID? Self-descriptive names are usually helpful :)
There was a problem hiding this comment.
That is the long standing id for the client that sent the message. Renaming is not going to happen. A message with a target client id uses the property name targetClientId.
| @@ -26,7 +38,12 @@ class PresenceManagerDataObject extends LoadableFluidObject { | |||
| if (!this._presenceManager) { | |||
| // TODO: investigate if ContainerExtensionStore (path-based address routing for | |||
| // Signals) is readily detectable here and use that presence manager directly. | |||
| this._presenceManager = createPresenceManager(this.runtime); | |||
| const manager = createPresenceManager(this.runtime); | |||
| this.runtime.on("signal", (message: IInboundSignalMessage, local: boolean) => { | |||
There was a problem hiding this comment.
Curious on why listening on events here? For a second I thought you are trying to decouple it from runtime, but runtime is still passed to presence manager.
There was a problem hiding this comment.
This is a move in that direction (to remove IFluidDataStoreRuntime from use). Eventually only IExtensionRuntime will be this package's connection to container runtime (IExtensionRuntime will probably be a subset of IContainerRuntime). But until container extension feature is implemented we are using IFluidDataStoreRuntime. One pattern that is different between connected as DataObject with IFluidDataStoreRuntime and connected as container extension is how inbound signals are sent. For container extension pattern, the extension [optionally] provides processSignal (see PresenceExtensionInterface). Here were are adapting DataObject to look more like the container extension host. So:
- listen to "signal" event here and call manager's
processSignal - do pass
IFluidDataStoreRuntimedown to manager but asIEphemeralRuntimewhich is the local definition of all of the things it needs fromIExtensionRuntimeeventually. You won't seeIExtensionRuntimeinIEphemeralRuntimebecauseIExtensionRuntimeis so unbaked as are the actual needs of presence.IContainerRuntimeis the temporary proxy forIExtensionRuntime.
Super clear, right? We made a number of compromises to be able to get the basic functionality out and work with system as it is. This entire file will eventually disappear.
to distinguish internal needs and external uses. IPresenceManager became three parts:
- IPresence (public user interface)
- PresenceManagerInternal (needs of internal presence components)
- PresenceExtensionInterface (for future container hostable)
- Push `on("signal"` up to temp DataObject and use `processSignal` for both paths.
- `submitSignal` still needs managed.
…s.ts to help avoid circular dependencies
jason-ha
force-pushed
the
presence/manager-interface-refactor
branch
from
50a57d1
to
b973533
Compare
to distinguish internal needs and external uses. IPresenceManager is removed (was accidentally duplicated) and became three distinct parts:
on("signal"up to temp DataObject and useprocessSignalfor both paths.submitSignalstill needs managed.Relocate IEphemeralRuntime to internalTypes.ts to help avoid circular dependencies.