App instance proposal + connect to app instance for intent results #287

nkolba · 2020-11-22T03:07:31Z

This is an initial PR to introduce the concept of AppInstances to address data returning intents - as outlined in issue #201 as well as questions around app instances (e.g. issue #231).

Key points of this PR are:

introduction of an AppInstance class that provides a reference to a specific instance of an app and allows direct broadcasting / listening of context with that instance
IntentResolution source properties can be used to obtain an AppInstance
ContextHandler functions now get an additional source arg that provides a reference to the instance sending the context or intent
Instances provide lifecycle information

mattjamieson

Great to start to define AppInstance as a concept, definitely seems like something we've been touching on more and more in conversations lately.

Added some comments inline.

mattjamieson · 2020-12-02T22:56:58Z

docs/api/ref/DesktopAgent.md

@@ -10,6 +10,7 @@ hide_title: true
 interface DesktopAgent {
  // apps
  open(name: string, context?: Context): Promise<void>;


Could open also return Promise<AppInstance>? Or at least an equivalent to the source property of an IntentResolution?

Yes that would make sense. I think returning the instance itself would be appropriate for open and consistent with window.open in the DOM.

@mattjamieson updated PR

mattjamieson · 2020-12-02T23:00:59Z

src/api/DesktopAgent.ts

+   * ```
+   * 
+   */
+  getAppInstance(instanceId : string ) : Promise<AppInstance>;


For getCurrentChannel we specify returning null if the channel doesn't exist (although the return type doesn't allow it, and I think there is a separate PR to fix that). Should we do that here too?:

getAppInstance(instanceId: string) : Promise<AppInstance | null>;

@mattjamieson after some more thought, I think we should leave as is. If the instance doesn't exist, you can still get an instance reference and get lifecycle events if it loads, set handlers on it, etc.

mattjamieson · 2020-12-02T23:05:14Z

docs/api/ref/DesktopAgent.md

+
+ Returns the instance of an app for a given identifier.
+
+  An instanceId can be obtained from


In #279 we're adding:

type Target = string | AppMetadata;

If/when we merge that we should clarify the language here as I think it only makes sense to getAppInstance with a string appId and not an AppMetadata object?

I think the instanceId here is different from AppMetadata being provided for the Target.

In the case of Target, we are addressing the app - regardless of instance - and matching on either name/appId or metadata and can return any instance (or even spawn a new one).

I agree - it's just that #279 changed IntentResolution.source to be of type Target so we probably need to be clear that this usage is only for the string variant.

Or:

type TargetSource = string; type Target = TargetSource | AppMetadata;

k. maybe it would make more sense if there was a type of InstanceId - instead of "TargetSource". e.g

type InstanceId = string; type Target = InstanceId | AppMetadata;

This would make the type names more aligned and accurate. (what you are targeting above would either be an exact InstanceId or a match on AppMetadata)

Target is actually set to change to TargetApp for clarity.

Might not be altogether cleaner to the instanceId to the AppMetadata object and retire the string? Or have an InstanceMetadata object that includes both the instanceId and AppMetadata.

I'm also wary of returning only the instanceId with no associated metadata identifying the app as no method is provided to retrieve said metadata for an AppInstance. Either the AppInstance itself should contain that or we should be returning it with the InstanceId as a metadata object.

mattjamieson · 2020-12-02T23:07:37Z

docs/api/ref/AppInstance.md

+
+  //methods
+  addContextListener(handler: ContextHandler): Listener;
+  addContextListener(contextType: string, handler: ContextHandler): Listener;


I still think we should fix the arg order for all the addContextListener methods 😁

They actually cause a lot of problems, being out of order. The code you have to write to deal with it is really ugly.

@rikoe @mattjamieson - if we made this change in 1.2, what would be the impact to existing apps? Can some of the platform owners weigh in? @kriswest @lspiro-Tick42 @tim-dinsdale

Speaking of jumping through hoops to make function signatures work, the API would be easier to implement and more consistent if addContextListener() were to return a Promise<Listener> rather than a Listener.

mattjamieson · 2020-12-02T23:08:39Z

docs/api/ref/AppInstance.md

+interface AppInstance {
+  //properties
+  readonly instanceId : string;
+  readonly status : 'ready' | 'loading' | 'unregistered';


Can we type the status?

type AppInstanceStatus = 'ready' | 'loading' | 'unregistered';

Yes. I was being lazy...

@mattjamieson updated PR

mattjamieson · 2020-12-02T23:09:13Z

docs/api/ref/AppInstance.md

+  addContextListener(handler: ContextHandler): Listener;
+  addContextListener(contextType: string, handler: ContextHandler): Listener;
+  broadcast(context: Context): void;
+  onStatusChanged(handler : (newVal : string, oldVal :string) => {}) : void;


As above:

onStatusChanged(handler : (newVal : AppInstanceStatus, oldVal :AppInstanceStatus) => void) : void;

@mattjamieson updated PR

rikoe

Looks good to me @nkolba - should we remove the data property from IntentResolution as part of this?

pgn-vole · 2020-12-15T18:50:25Z

docs/api/spec.md

+Using FDC3 APIs, an app can get a reference to an instance of another app that it has either raised an intent to or recieved an intent or context from.  The `AppInstance` API allows apps to listen for and broadcast context directly.  
+
+### Broadcasting and Listening for Context
+An AppInstance MUST support both *broadcast* and *addContextListener* methods.  Calling *broadcast* on an AppInstance MUST send the context only *to* that instance.  The *addContextListener* method MUST listen for context events only *from* that instance. 


addContextListener method MUST listen for context events only from that instance.

How can the application instance, when using fdc3.broadcast, differenciate between broadcasting to a channel, that it has joined previously, and broadcasting to an actual "appInstance listener"?
I understand that the application instance can do that differenciation with the fdc3.addContextListener API because the handler does provide a source property, which hints on wether the new context is or is not coming from a channel broadcasting. But it is not clear on how this distinction can be done with fdc3.broadcast.

@pgn-vole instance messages can only be handled/passed via addContextListener and broadcast on the instance. So, fdc3.broadcast would never get routed to an instance.addContextListener handler and vice-versa. This is consistent with broadcast/handler behavior on channels. The only exception being when an app has actually joined a channel - in which case - the channel context is the same as the global broadcast. I will look amending the spec to make that more clear.

BTWY - The source property won't provide this differentiation - you will always have a single instance source - whether this is coming from a channel, a direct message from an instance, or a global broadcast. The distinction is only in where the app sets the listener.

Thanks for the explanation. So for an application instance to broadcast a message to any of its listeners it must beforehand retrieve its own instance and then broadcast via it like the below?

Listener:

const chart = await fdc3.raiseIntent('ViewChart'); const instance = await fdc3.getAppInstance(chart.source); instance.addContextListener("someContext", handler);

"ViewChart" Application:

const instance = await fdc3.getAppInstance(myInstanceId); // myInstanceId is exposed in some way by the desktop agent instance.broadcast({type: "someContext"}) //broadcast its app instance listeners fdc3.broadcast({type: "someContext"})// broadcast to currently joined channel

Not exactly. An app shouldn't broadcast on its own instance - but instead, broadcast on the instance of a subscribing app (retrieved from the intentListener or contextListener). So, in the "ViewChart" example above, you would do this instead:

fdc3.addIntentListener("ViewChart",async (context, source) => { const sourceInstance = await fdc3.getAppInstance(source); sourceInstance.broadcast({type: "someContext"}) });

Thanks that makes more sense. I got confused by the section "Context last value cache" which made me assume that there would be a single context per application instance, however it seems that instead there will be a context held for each relation provider/subscriber.

We had similar issues with initial comprehension here, due to the slightly unconventional (but perhaps necessary) approach.

@nkolba When you say that (the app should) "broadcast on the instance of a subscribing app (retrieved from the intentListener or contextListener)" how does it do that retrieval? It would have needed to register a listener of some sort for the addition of those listeners in order to retrieve their IDs. That seems too complex to be practical.

Perhaps what @pgn-vole suggests is simpler (retrieving its own AppInstance and broadcasting on that) OR perhaps some object is returned by addIntentListener for that purpose (this does not solve the open case however).

…w resolves with an app instance. Fixed contextHandler type

sean-ciq · 2021-01-26T16:54:06Z

docs/api/ref/AppInstance.md

+
+AppInstances can be used to directly listen for and broadcast context between two apps connected to a desktop agent.
+
+An AppInstance is obtained by calling the `getAppInstence` method on the `DesktopAgent` using a source token provided by either an IntentResolution or as an argument to a ContextHandler. 


just a small typo: getAppInstence should be getAppInstance

kriswest

This is a great start @nkolba.

The biggest outstanding issues I/we see are:

the approach is unconventional (though necessary), leading to some problems with comprehension.
- This might be resolved through documentation and naming conventions (e.g. replacing source in IntentResolution with appInstanceId.
The return types of open and raiseIntent are inconsistent. open returns a Promise<AppInstance | null> while raiseIntent returns an IntentResolution containing a source string (presumed to have changed from the apps name to appInstanceId). These should be harmonized.
There is no way to retrieve the AppMetadata associated with an InstanceId - this could be solved by:
- including the AppMetadata in the AppInstance objects returned
- OR by adding the InstanceId to the AppMetadata object and returning that
- OR by creating an InstanceMetadata object that wraps the InstanceId and an AppMetadata object
There is some inconsistency arising from the fact that raiseIntent accepts a Target (see Adds a new Target type, updates open() and raiseIntent() #279) to be renamed TargetApp (feat: rename Target to TargetApp and update docs to match API #315) but broadcast and addContextListener are called on the AppInstance.
- The proposal should incorporate the merged changes in Adds a new Target type, updates open() and raiseIntent() #279 and proposed improvement in feat: rename Target to TargetApp and update docs to match API #315
- we should consider whether it would be more consistent to:
  - further refine the TargetApp to allow use of InstanceMetadata or an AppMetadata with instance id
  - OR allow raiseIntent on the AppInstance object (as the proposal does with broadcast and addContextListener)
  - OR add a target option to the desktop agent's broadcast and addContextListener functions instead of creating an AppInstance object at all (i.e. redefine this proposal using instance metadata only and not an object.
The proposed method of responding to an app that has called addContextListener on an AppInstance (retrieving that app's own AppInstance and broadcasting to it) has issues.
- The responding app currently has no way to get the InstanceId of the subscribing app in order to retrieve it.
  - In the case of fdc3.open, to resolve the open issues, an app would have to register a listener for subscriptions and then retain Ids or AppInstance objects to use for this purpose.
  - In the case of fdc3.raiseIntent the ContextHandler argument on addIntentListener would need to have an additional argument added to it to receive the source instanceId.
  - This feels far too complex to be practical.
- As this is the primary usecase being addressed this is perhaps the most important issue to address in the proposal.

kriswest · 2021-02-12T09:37:07Z

docs/api/ref/DesktopAgent.md

+
+ Returns the instance of an app for a given identifier.
+
+  An instanceId can be obtained from


Target is actually set to change to TargetApp for clarity.

Might not be altogether cleaner to the instanceId to the AppMetadata object and retire the string? Or have an InstanceMetadata object that includes both the instanceId and AppMetadata.

I'm also wary of returning only the instanceId with no associated metadata identifying the app as no method is provided to retrieve said metadata for an AppInstance. Either the AppInstance itself should contain that or we should be returning it with the InstanceId as a metadata object.

kriswest · 2021-02-12T09:38:51Z

src/api/AppInstance.ts

+ * An interface that relates an instance of an app to other apps
+ */
+export interface AppInstance {
+  readonly instanceId: string;


Consider augmenting this interface with AppMetadata to identify the app that this is an instance of.

The AppMetadata could wrap the instanceId or be a sibling of it

kriswest · 2021-02-12T09:43:06Z

src/api/methods.ts

+export const open: (
+  name: string,
+  context?: Context
+) => Promise<AppInstance | null> = (name, context) => {


This is inconsistent with IntentResolution which only returns a source (whose value changes, I assume from the app name (which was not sufficient to uniquely identify an app in either a single or multiple appDs anyway) to an instanceId and requires you to getAppInstance. Either both should return the InstanceId or both should return a Promise<AppInstance | null>

Intent resolution was handled in this way to preserve backwards compatibility. Given that this change is being pushed to a 2.x, then breaking changes to IntentResolution could be on the table. Have the intent resolve to an AppInstance (consistent with open) seems to make sense.

kriswest · 2021-02-12T10:53:09Z

docs/api/spec.md

-// do some vendor-specific stuff
+
+//get an AppInstance object back from the DesktopAgent
+const instance = await fdc3.getAppInstance(chart.source);


redefines the meaning of IntentResoluton.source to an InstanceId (currently an app 'name'). Consider adding a different field and/or metadata object for this.

see comment above. Also, note that 'source' was never specifically defined as app 'name' - left up to implementation by design.

kriswest · 2021-02-12T11:03:27Z

docs/api/spec.md

+Using FDC3 APIs, an app can get a reference to an instance of another app that it has either raised an intent to or recieved an intent or context from.  The `AppInstance` API allows apps to listen for and broadcast context directly.  
+
+### Broadcasting and Listening for Context
+An AppInstance MUST support both *broadcast* and *addContextListener* methods.  Calling *broadcast* on an AppInstance MUST send the context only *to* that instance.  The *addContextListener* method MUST listen for context events only *from* that instance. 


We had similar issues with initial comprehension here, due to the slightly unconventional (but perhaps necessary) approach.

@nkolba When you say that (the app should) "broadcast on the instance of a subscribing app (retrieved from the intentListener or contextListener)" how does it do that retrieval? It would have needed to register a listener of some sort for the addition of those listeners in order to retrieve their IDs. That seems too complex to be practical.

Perhaps what @pgn-vole suggests is simpler (retrieving its own AppInstance and broadcasting on that) OR perhaps some object is returned by addIntentListener for that purpose (this does not solve the open case however).

kriswest · 2021-02-15T08:56:15Z

On further review, we also note that there is a danger of an app losing the association of a raised intent with a response with this approach, which implements communications between two instances after an intent is raised, but doesn't necessarily relate those comms to a specific intent. What happens if an app raises more than one intent and targets the same instance? The same conduit would have to be used to reply to both intents, with messages potentially being interleaved and needing additional context metadata to relate them to the specific intents.

However, there already exists the concepts of app channels that could be used to implement the comms between instances. As an application can create and join more than one channel it can uniquely tie a stream of responses to a single event. Alternatively, it can also reuse a channel (e.g. if more than one app requests the same pricing stream).

Using channels would also eliminate the duplication of functionality that the proposed AppInstances create (you can already broadcast and addContextListener on the desktop agent and channels, this proposal would create a third set of these functions). Working with a channel returned by raiseIntent or open would also be easier to understand conceptually.

rikoe · 2021-10-11T11:10:10Z

@nkolba we propose closing this PR as it has been inactive for a while, and due to the comments above, and potential issues that was raised - which do not have a good solution at the moment - we were not able to reach consensus at the Standard Working Group on accepting this PR.

We will continue to explore ways to achieve the proposals raised in this PR in the bi-weekly Channels & Feeds Discussion Group, please feel free to participate in those discussions.

kriswest · 2021-11-12T11:41:01Z

Superceded by:

final draft additions for AppInstance spec

34295a0

finos-cla-bot bot added the cla-present label Nov 22, 2020

nkolba requested review from rikoe, kriswest, lspiro-Tick42 and donbasuno November 22, 2020 03:07

This was referenced Nov 22, 2020

Data intents finos/fdc3-desktop-agent#21

Closed

Data Driven Intents #201

Closed

mattjamieson reviewed Dec 2, 2020

View reviewed changes

rikoe reviewed Dec 3, 2020

View reviewed changes

pgn-vole reviewed Dec 15, 2020

View reviewed changes

nkolba mentioned this pull request Dec 21, 2020

Data intents finos/fdc3-desktop-agent#22

Merged

nkolba added 3 commits December 23, 2020 16:32

updates based on feedback. Added AppInstanceStatus enum. fdc3.open no…

f41e529

…w resolves with an app instance. Fixed contextHandler type

Merge branch 'master' into data-intents

4c050bb

formatting

d01e5e6

rikoe mentioned this pull request Jan 7, 2021

Standard WG Meeting - Jan 7, 2020 #298

Closed

19 tasks

sean-ciq reviewed Jan 26, 2021

View reviewed changes

rikoe added this to the 2.0 milestone Jan 26, 2021

rikoe mentioned this pull request Jan 27, 2021

Standard WG Meeting - Jan 27, 2020 #303

Closed

6 tasks

rikoe added the api FDC3 API Working Group label Jan 27, 2021

kriswest requested changes Feb 12, 2021

View reviewed changes

rikoe changed the title ~~final draft additions for AppInstance spec~~ App instance proposal with ability to connect to app instances for listening to intent results Apr 8, 2021

rikoe changed the title ~~App instance proposal with ability to connect to app instances for listening to intent results~~ App instance proposal + connect to app instance for intent results Apr 8, 2021

kriswest mentioned this pull request Apr 22, 2021

Make it possible to implement transactions and feeds (aka data driven intents, RPC etc.) via existing FDC3 concepts #372

Closed

kriswest mentioned this pull request Sep 30, 2021

Channels, Feeds & Transactions Discussion group meeting 30th Sept 2021 #465

Closed

14 tasks

kriswest closed this Nov 12, 2021


		Returns the instance of an app for a given identifier.

		An instanceId can be obtained from


		AppInstances can be used to directly listen for and broadcast context between two apps connected to a desktop agent.

		An AppInstance is obtained by calling the `getAppInstence` method on the `DesktopAgent` using a source token provided by either an IntentResolution or as an argument to a ContextHandler.

App instance proposal + connect to app instance for intent results #287

App instance proposal + connect to app instance for intent results #287

Conversation

nkolba commented Nov 22, 2020

mattjamieson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjamieson Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikoe left a comment

Choose a reason for hiding this comment

pgn-vole Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgn-vole Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriswest left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriswest commented Feb 15, 2021

rikoe commented Oct 11, 2021

kriswest commented Nov 12, 2021

mattjamieson Dec 3, 2020 •

edited

Loading

pgn-vole Dec 15, 2020 •

edited

Loading

pgn-vole Jan 6, 2021 •

edited

Loading

kriswest left a comment •

edited

Loading