Argument order in addContextListener overloads complicates implementation

[mattjamieson]

Area of Issue

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

Issue Description:

The argument order in the two addContextListener overloads of DesktopAgent complicates implementation.

    addContextListener(handler: ContextHandler): Listener;
    addContextListener(contextType: string, handler: ContextHandler): Listener;

Means you have to use union types to implement in TypeScript:

    addContextListener(a: ContextHandler | string, b?: ContextHandler): Listener {
        var handler = a as ContextHandler;
        var contextType = null;

        if (typeof a !== "function") {
            contextType = a as string;
            handler = b as ContextHandler;
        }

        ...    

I suggest we flip the argument order in the second version:

    addContextListener(handler: ContextHandler): Listener;
    addContextListener(handler: ContextHandler, contextType: string): Listener;

[mattjamieson]

Or make the second argument optional...

 addContextListener(handler: ContextHandler, contextType?: string): Listener;

[rikoe]

This is definitely an oversight/error that we should fix ASAP @nkolba. Thanks @mattjamieson !

[nkolba]

@mattjamieson @rikoe . Looking at this issue again... While these are valid points raised by Matt, this is not actually an oversight - but by design. The signature is made to be backwards compatible with FDC3 1.0 (which only had the listener argument) as well as symmetrical with addIntentListener(intent, handler). So, while maybe far from perfect - it was definitely a choice.

I do think we'd benefit from some rethinking of the APIs for handling context and intent events in the not so distant future. The landscape has changed with channels in the mix and we have learned a lot from implementations of 1.0 and 1.1. There is also a lot of grey area between intent and context handling that could be better handled.

TLDR; my 2 cents would be to leave this as is for now and overhaul the event handling APIs (add new ones, deprecate the old) in 1.2+.

[ryanswanson]

We recently began adoption of FDC3 and encountered this issue, having implemented similar argument handling in TypeScript as described by @mattjamieson in the original posting above.

I noticed this has been added to the 2.0 milestone recently and wanted to share a rationale that supports the suggested signature change above: addContextListener(handler: ContextHandler, contextType?: string): Listener;.

First, it seems the addition of contextType as a second, optional parameter could still be considered backwards compatible between FDC3 1.0 and 1.1. This would now be a breaking change to switch order since the optional contextType is listed first in 1.1, but perhaps that is why this issue is slated for 2.0?

From an API design perspective, a difference (asymmetry) between the addIntentListener and addContextListener method signatures could be considered appropriate since their function is innately dissimilar.

• Adding contextType to the addContextListener method allows for more granular subscription within the same Context domain, thus contextType is optional.
• By contrast, addIntentListener subscribes based on intentName (Intent domain) and associates to a Context handler, thus intentName is required.

TLDR; The differences in argument optionality due to correlation between domains can justify a difference in signatures. Also, providing optional parameters last within a method signature is a generally preferred convention over method overloading in JavaScript/TypeScript.

Hope this feedback is helpful.

[rikoe]

Thanks @ryanswanson - this is very helpful. We are indeed considering this change for v2.0 as a breaking change.