DesktopAgent: Method generation creates multiple overloads

[Anlanther]

Minor Issue

Area of Issue

[ ] App Directory
[x ] API
[ ] Context Data
[ ] Intents
[ ] Use Cases
[ ] Other

Issue Description:

When implementing the DesktopAgent on another class, automatic method generation leads to multiple overloads, such as with the open() and raiseIntent() methods. The generated methods also include inline imports and when other method overloads are removed to match documentation, there are incompatibility issues.

Additional Context:

This is what is generated when implementing the DesktopAgent on a fresh new class and what also happens when starting a project from scratch (Angular). As can be seen, there are overloads for methods such as open() and raiseIntent(). Both also include an odd inline import (Promise<import("@finos/fdc3").AppIdentifier>):

import { AppIdentifier, AppIntent, AppMetadata, Channel, Context, ContextHandler, DesktopAgent, ImplementationMetadata, IntentHandler, IntentResolution, Listener, PrivateChannel } from "@finos/fdc3";

export class ExampleDesktopAgent implements DesktopAgent{
  open(app: AppIdentifier, context?: Context | undefined): Promise<AppIdentifier>;
  open(name: String, context?: Context | undefined): Promise<AppIdentifier>;
  open(name: unknown, context?: unknown): Promise<import("@finos/fdc3").AppIdentifier> {
    throw new Error("Method not implemented.");
  }
  findIntent(intent: string, context?: Context | undefined, resultType?: string | undefined): Promise<AppIntent> {
    throw new Error("Method not implemented.");
  }
  findIntentsByContext(context: Context, resultType?: string | undefined): Promise<AppIntent[]> {
    throw new Error("Method not implemented.");
  }
  findInstances(app: AppIdentifier): Promise<AppIdentifier[]> {
    throw new Error("Method not implemented.");
  }
  broadcast(context: Context): Promise<void> {
    throw new Error("Method not implemented.");
  }
  raiseIntent(intent: string, context: Context, app?: AppIdentifier | undefined): Promise<IntentResolution>;
  raiseIntent(intent: string, context: Context, name: String): Promise<IntentResolution>;
  raiseIntent(intent: unknown, context: unknown, name?: unknown): Promise<import("@finos/fdc3").IntentResolution> {
    throw new Error("Method not implemented.");
  }
  raiseIntentForContext(context: Context, app?: AppIdentifier | undefined): Promise<IntentResolution>;
  raiseIntentForContext(context: Context, name: String): Promise<IntentResolution>;
  raiseIntentForContext(context: unknown, name?: unknown): Promise<import("@finos/fdc3").IntentResolution> {
    throw new Error("Method not implemented.");
  }
  addIntentListener(intent: string, handler: IntentHandler): Promise<Listener> {
    throw new Error("Method not implemented.");
  }
  addContextListener(contextType: string | null, handler: ContextHandler): Promise<Listener>;
  addContextListener(handler: ContextHandler): Promise<Listener>;
  addContextListener(contextType: unknown, handler?: unknown): Promise<import("@finos/fdc3").Listener> {
    throw new Error("Method not implemented.");
  }
  getUserChannels(): Promise<Channel[]> {
    throw new Error("Method not implemented.");
  }
  joinUserChannel(channelId: string): Promise<void> {
    throw new Error("Method not implemented.");
  }
  getOrCreateChannel(channelId: string): Promise<Channel> {
    throw new Error("Method not implemented.");
  }
  createPrivateChannel(): Promise<PrivateChannel> {
    throw new Error("Method not implemented.");
  }
  getCurrentChannel(): Promise<Channel | null> {
    throw new Error("Method not implemented.");
  }
  leaveCurrentChannel(): Promise<void> {
    throw new Error("Method not implemented.");
  }
  getInfo(): Promise<ImplementationMetadata> {
    throw new Error("Method not implemented.");
  }
  getAppMetadata(app: AppIdentifier): Promise<AppMetadata> {
    throw new Error("Method not implemented.");
  }
  getSystemChannels(): Promise<Channel[]> {
    throw new Error("Method not implemented.");
  }
  joinChannel(channelId: string): Promise<void> {
    throw new Error("Method not implemented.");
  }
}

Example error when removing i.e. open() overloads...
Removed:
open(name: String, context?: Context | undefined): Promise<AppIdentifier>;
open(name: unknown, context?: unknown): Promise<import("@finos/fdc3").AppIdentifier>

Kept:
open(app: AppIdentifier, context?: Context | undefined): Promise<AppIdentifier>;

Result:

Property 'open' in type 'ExampleDesktopAgent' is not assignable to the same property in base type 'DesktopAgent'.
  Type '(app: AppIdentifier, context?: Context | undefined) => Promise<AppIdentifier>' is not assignable to type '{ (app: AppIdentifier, context?: Context | undefined): Promise<AppIdentifier>; (name: String, context?: Context | undefined): Promise<...>; }'.
    Types of parameters 'app' and 'name' are incompatible.
      Property 'appId' is missing in type 'String' but required in type 'AppIdentifier'.

Also had to replace all generated String with string.

[kriswest]

The overloads are all for deprecated functions, which you could remove as they are inherently optional (although that may indeed cause typing issues). We can't remove them from the FDC3 Standard until a later major release, however.

Given that you have top-level imports for the types that have weird inline imports I would look to whatever code generation tool you are using for a solution to that issue.

There are indeed some capitalized Strings in the DestkopAgent types! They appear to only be in the deprecated functions:

FDC3/src/api/DesktopAgent.ts

Lines 546 to 566 in e8d0248

	open(name: String, context?: Context): Promise<AppIdentifier>;
	/**
	* @deprecated version of `raiseIntent` that targets an app by by name rather than `AppIdentifier`. Provided for backwards compatibility with versions FDC3 standard <2.0.
	*
	* ```javascript
	* // use the `name` metadata of an app to describe the target app for the intent
	* await fdc3.raiseIntent("StartChat", context, appIntent.apps[0].name);
	* ```
	*/
	raiseIntent(intent: string, context: Context, name: String): Promise<IntentResolution>;
	/**
	* @deprecated version of `raiseIntentForContext` that targets an app by by name rather than `AppIdentifier`. Provided for backwards compatibility with versions FDC3 standard <2.0.
	*
	* ```javascript
	* // Resolve against all intents registered by a specific target app name for the specified context
	* await fdc3.raiseIntentForContext(context, targetAppName);
	* ```
	*/
	raiseIntentForContext(context: Context, name: String): Promise<IntentResolution>;

A PR should be raised to fix that.

Do you have any other concrete asks (apart from removal of the overloads, which we can't do soon due to our governance/versioning policy)?

[novavi]

@Anlanther I agree with what @kriswest said about the how it's notable that the weird inline imports already have top-level imports for those types. The behaviour you're experiencing must be a result of the IDE / TypeScript config that you're using (whether explicitly or implicitly). I don't experience the weird inline imports when I implement the interface in my environment, so I do not believe this is a bug in @finos/fdc3.

I can't speak with authority on this, but as for the apparent need to remove method overloads after implementing the interface in order to match the documentation, I'm not so sure I personally agree with this as a strategy if you were following the FDC3 2.0 spec.

The overloads (multiple declarations immediately followed by the same-named implementation) are perfectly valid TypeScript syntax, and they are there for a reason - they clearly show the current and the deprecated function signatures that are included in the FDC3 2.0 spec.

So if implementing the DesktopAgent interface you must fill in the function implementation and use the args passed to it at runtime in order to satisfy the required behaviour of the current signature (and you may also satisfy the behaviour for the deprecated signature).

If you are in fact implementing a desktop agent in a browser (whether for demo or other purposes), personally I think it would be good practice to:

• separate the desktop agent concept from your application code
• have your desktop agent install itself in the window.fdc3 object
• use the @finos/fdc3 wrapper functions in your application - these will call through to your implementation in the window.fdc3 object.

This would make the boundaries between the desktop agent and the application clearer. It would also provide clarity on whether your agent and application were using the current or the deprecated versions of the various FDC3 methods (because you'd benefit from the @deprecated flags included in the interface). Finally, it could also go some way to keeping you honest e.g. if in future someone within your team tried to bastardise your agent implementation by adding proprietary non-standard methods, these would not be accessible from your application using the @finos/fdc3 wrapper functions, and the separation would hopefully make it more obvious that they were attempting something in the wrong way.

As @kriswest pointed out, the overloads could potentially be removed in future versions of the spec (i.e. beyond 2.0). But right now, the spec clearly establishes current and deprecated signatures - and since TypeScript supports overloaded class method declarations and overloaded function declarations, I believe you should be able to implement the DesktopAgent interface without needing to manually remove any of the signatures that get autogenerated by your IDE when implementing the interface.

I actually think in your specific case the weird inline import syntax clouded the issue. I believe it was the confusion around equivalence of the Promise<AppIdentifier> return type used in your method declarations and the Promise<import("@finos/fdc3").AppIdentifier> return type used in your method implementation that led to the error you experienced. If you can resolve whatever is causing the weird inline import problem in your environment, then you should not need to manually remove or change any of the signatures.

Related to this, there's currently an inconsistency between the method signatures in DesktopAgent.ts and the function signatures in Methods.ts. As you've seen, the former contains the overloads, whereas the latter omits them. FYI there's some discussion on this - and an example of overload declarations followed by an implementation - here:
FDC3 2.0 questions

[kriswest]

@Anlanther We believe the questions in this issue are answered. Hence, closing this issue - feel free to reopen if you have further comments. Please note, we've raised a separate issue to deal with the use of the String Object type in deprecated FDC3 functions and will correct them to string in the next version:

• API types use String rather than string #915