Conversation
✅ Deploy Preview for fdc3 canceled.
|
|
|
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
I'll try and take a look at this and the conformance PR. I do think this needed a refactor. However, if this implementation starts seeing use in the wild, then this is a big breaking change and needs to go into a major release of the associated library... There may already be others depending on the code... I can see an argument that it might need its own versioning and project... We'll need to decide what to do and future governance.
I don't think the changes conflict in a big way with #1695 (the reference implementation should not be using a websocket when it doesn't need to) - hopefully just small resolvable conflicts. It will conflict with a PR I haven't got around to raising yet but is WIP on context cleared events - should have got it in earlier!
It'd be good to have in issue raised for the proposed changes wherever possible.
Its good to see a bit more documentation in this version - however, these changes make the README.md in /toolbox/fdc3-for-web/fdc3-web-impl in correct and it'll need a new write-up.
|
|
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
Apparently I abandoned a review last year - closing it to drop comments I'd left you @robmoffat
| * Handles messaging to apps and opening apps for ONE FDC3 environment. | ||
| * Stores all state for MessageHandlers to use. | ||
| */ | ||
| export interface FDC3ServerInstance { |
There was a problem hiding this comment.
FDC3ServerInstance suggests a concrete implementation rather than an interface... Perhaps better to stick to FDC3Server
There was a problem hiding this comment.
I appreciate that the demo server can start multiple instances, which recycle the same socket.io server, with different instance IDs, but that feels like an oddity rather than a pattern; it probably doesn't make sense for end-users to have multiple different instances of a DA that are each an island of interop. I imagine most implementors will find a way to have multiple windows talk to the same Desktop Agent.
Server/Client terminology is perhaps also not helping here (too many servers... and this one is actually running in a browser window). I wonder if something like DesktopAgentCore or DesktopAgentBackend would be easier to understand/intuit?
| * Subclass this and implement the createInstance method with your | ||
| * implementation of FDC3ServerInstance. | ||
| */ | ||
| export abstract class AbstractFDC3ServerFactory { |
There was a problem hiding this comment.
What advantage is there to having this factory pattern, separate from an Implementation of an FDC3ServerInstance? The constructor it provides is opinionated about what it sets up (such as the handlers that would be used in instances). Why don't we just have an FDC3Server interface and a default implementation of it that sets up the handlers (which again conform to an interface)?
The example in demo put both the factory and instance implementations in the same file and the factory is just passing on args to the instances' constructor. Why not just construct instances directly, or even just have a static createInstance function on an FDC3Server implementation (if you regularly need to create new instances - which I don't actually think is the case. That way the implementation is on control of what handlers etc. that it sets up - that is exactly the sort of thing that is likely to vary between implementations (e.g. a firm using this needs to provide a custom IntentHandler in order to tie it into some other subsystem).
| import { PendingApp } from './PendingApp'; | ||
| import { Directory } from './directory/DirectoryInterface'; | ||
|
|
||
| export enum HeartbeatActivityEvent { |
There was a problem hiding this comment.
might be worth extracting these types to a new file and consolidating with AppRegistration and any others
| @@ -48,19 +51,23 @@ function isRunningAppRegistration( | |||
| return !!(details as RunningAppRegistration).window; | |||
| } | |||
There was a problem hiding this comment.
Extracting the types and typeguards above into a separate file would make this easier to read
| setInstanceDetails(uuid: InstanceID, meta: RunningAppRegistration | LaunchingAppRegistration): void { | ||
| setInstanceDetails(uuid: InstanceID, meta: AppRegistration): void { | ||
| // Type assertion to handle our extended type | ||
| const demoMeta = meta as DemoAppRegistration; |
There was a problem hiding this comment.
In the previous pattern a generic was used in the ServerContext to allow implementations to customise the registration - that felt better than having overrides.
| createInstance(): FDC3ServerInstance { | ||
| return new DemoFDC3ServerInstance(this.socket, this.directory, this.handlers, this.channels); |
There was a problem hiding this comment.
Example of this not providing any tangible benefit - its just separating default values and behaviours over 3 files making them harder to interpret. I'm not seeing an advantage, just extra complexity.
| contextType: string | null; | ||
| }; | ||
|
|
||
| export type PrivateChannelEventListener = { |
There was a problem hiding this comment.
To be consistent with the others, this type and DesktopAgentEventListener should end in Registration
| export type PrivateChannelEventListener = { | |
| export type PrivateChannelEventListenerRegistration = { |
| /** | ||
| * Return the list of all apps that have ever been registered with the ServerContext. | ||
| */ | ||
| getAllApps(): Promise<AppRegistration[]>; |
There was a problem hiding this comment.
To avoid confusion with retrieving app details bring 'Instance' or 'Registration' into the name:
| getAllApps(): Promise<AppRegistration[]>; | |
| getAllAppRegistrations(): Promise<AppRegistration[]>; |
| getConnectedApps(): Promise<AppRegistration[]>; | ||
|
|
||
| /** | ||
| * Return the list of all apps that have ever been registered with the ServerContext. |
There was a problem hiding this comment.
I'd also consider updating this comment to remove 'ever been' as a long running implementation might drop older registrations, or ones it knows are gone forever (say a window closed so there no way its coming back as the same instance ever again - the instance details in SessionStorage dies with the window). Also clarify that its instances/registrations rather than app definitions.
| * Return the list of all apps that have ever been registered with the ServerContext. | |
| * Return the list of all app instances that have been registered with the ServerContext. |
| * Note, it is the implementor's job to ensure this list is | ||
| * up-to-date in the event of app crashes or disconnections. | ||
| */ | ||
| getConnectedApps(): Promise<AppRegistration[]>; |
There was a problem hiding this comment.
| getConnectedApps(): Promise<AppRegistration[]>; | |
| getConnectedAppRegistrations(): Promise<AppRegistration[]>; |
See later comment getAllApps(), rename will help comprehension. Mmatters less here because of the 'connected', but we should be consistent.
|
@robmoffat Not sure why you are removing the fdc3-web-impl... I thought we were going to keep it but mark not for production use and encourage sail to fork off and create and maintain a production ready version? |
2 participants
Describe your change
MessageHandlers,ServerContextandFDC3Serverby combiningServerContextandFDC3ServerintoFDC3ServerInstance, with an abstract implementation re-used infdc3-get-agentandfdc3-web-impltests and thedemodesktop agent itself.FDC3ServerInstanceclass, giving a better separation of concerns withMessageHandlers: these only manage conversion to and from the DACP / WCP format.HeartbeatHandlerand intoFDC3ServerInstancevia theheartbeatActivitymethod. This means that desktop agent implementations extendingFDC3ServerInstanceare able to customise the connection / disconnection process themselves if they think they have a better idea of the state of applications.Related PR
This is based on #1586 which should be merged and released first as it contains fixes for
fdc3-agent-proxy.Contributor License Agreement
Review Checklist