Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modifications to the Channels API + Window selectors. #121

Closed
chedzoir opened this issue Oct 16, 2019 · 13 comments
Closed

Modifications to the Channels API + Window selectors. #121

chedzoir opened this issue Oct 16, 2019 · 13 comments
Labels
api FDC3 API Working Group

Comments

@chedzoir
Copy link

There a couple of alterations that I'd suggest making to the API following my attempts at implementing it within our setup and how we expect it will actually get used

  1. Can the default channel be be hidden behind a getter that returns a promise. That would make it's use consistent with the getOrCreate and getSystemChannels.

  2. Could we modify the addBroadcastListener to take an optional array of context types. Then the interop broker would only notify the listener in the event of one of those specified types being transmitted.
    This gives the following advantages

  • As it's possible to put anything on a channel, receiving applications will all have to put in their place their own filtering mechanisms to only receive the items that they
  • Allows applications to put different context types onto a channel that can be picked up by different consuming applications. For example, two different instruments can be transmitted by app and treated in different ways by a number of different receiving apps - some may want both, some one, some the other.
    There is a downside here in that the channel will need to keep a history of the last received item by type, instead of just the last item - but that would just be a case of holding them within a map
  1. Following on from 2, we would then need to add the same context type to the getCurrentContext method

  2. In context of a requirement that we have with regards to window based channel selectors, and following on from https://github.com/FDC3/FDC3/issues/118 I would propose the following approach to managing window selectors

On the channels module in https://github.com/FDC3/FDC3/blob/master/src/api/channel/channel.ts
add a function something along the lines of

public addWindowChannelListener(listener: (event: {channel: Channel; context: Context}) => void, types: string[]): DesktopAgent.Listener;

Then the application does not need to know about or worry about the action of selecting a channel. The swapping between channels can be handled within the broker/router code.

If the user sets the channel to "off" then the application just doesn't receive anything
A DesktopAgent.Listener is returned should the application decide that it wishes explicitly stop receiving anything regardless.

@pjbroadbent
Copy link

Context filtering
In terms of adding a context argument to addBroadcastListener, that could certainly be useful - but would make it inconsistent with addIntentListener - would we also add a context argument to intent listeners too? I'm also assuming you're referring to both the "channel" addBroadcastListener and the "top level" addBroadcastListener?

There's also an implicit assumption here that context filtering would only ever happen on the type field - that application's won't want to say "I'll allow any context type that has an ISIN code in it's id section", for example. To clarify I'd prefer context filtering to be entirely type-based, as implied by the proposed changes, but worth considering any side-effects of that approach.

Window-level context listeners
If I'm understanding things correctly, the described purpose of the addWindowChannelListener is exactly how the current channels API proposes that the top-level addBroadcastListener works. If you call addBroadcastListener on a channel object, your listener will be called whenever there's a broadcast on that specific channel. If you call the top-level addBroadcastListener, then that listener will be called whenever there's a broadcast on whichever channel that window currently belongs to. The exception would be if #118 is accepted - then any windows in the 'off' channel would never have their top-level context listeners called, even if another window in the 'off' channel were to broadcast.

This is the main way channels are intended to be used - applications will typically only use the top-level addBroadcastListener, and then use getSystemChannels/(get)defaultChannel and join to control their membership - applications can switch between channels without having to remove and re-add listeners every time they change channel. This has the added benefit of being able to use channels with a window that has no knowledge of the channels API, so long as there is some external channel manager application, or some kind of default window chrome (with a channel selector) attached to the window.

@rikoe rikoe added the api FDC3 API Working Group label Oct 16, 2019
@rikoe
Copy link
Contributor

rikoe commented Oct 16, 2019

I am highly supportive of adding context types to the broadcast listener, in fact I think I have suggested it more than once in the past.

It also provides an opportunity for optimisation, as underlying messages can now be filtered down to only those ones applications are interested in.

I personally prefer adding a single context type parameter, and using separate subscriptions if multiple types are required, as I think it makes for nicer consumption, but I am willing to be persuaded 😄

Also, 👍 on only supporting filtering by context type - it is the organising principle of FDC3, also for intents.

I don't think adding context type filtering to broadcast implies it needs to be added to intents. Intents already support filtering context type in the app directory registration, and would just add unnecessary complexity for not a lot of gain.

@chedzoir can you please add some before and after straw man code samples (of usage) to this issue?

@chedzoir
Copy link
Author

chedzoir commented Oct 16, 2019

@pjbroadbent
Re addBroadcastListener, based on earlier conversations I was under the impression that that was going to be deprecated and the behaviour moved fully into the channels api, which is where we have a gap.
If we're keeping addBroadcastListener and if we clarify that it works on either a predefined channel or the selected window channel, then I think all is good. Then there's nothing to stop that having the same behaviour, so if the user selects off then the desktop agent doesn't call the listener nor does it react to broadcast.
Then there's no need to add an additional function.

I'm not sure that you need to have the getSystemChannels used in this context. The addBroadcastListener should hide that away for the average app.

@vmehtafds
Copy link

@chedzoir - My understanding is that most normal applications should be unaware of channels. The channels logic and switching to off or any System channel should be outside the application. The app code itself shouldn't do anything when a user put that app in the off or any colored channel. The channel management would be handled externally (may be in frame around the app or an external application).

Thus expecting an app to remove the top level listener or not react to broadcast when it is in off channel means app need to know whether it is in an off channel. This goes against the idea that I think previously most people had agreed to that ideally an app would be unaware of channels.

@chedzoir
Copy link
Author

chedzoir commented Dec 5, 2019

@vmehtafds - That's not at all what I mean. To clarify, I would expect that there is a method that the app has to know about that would allow it to subscribe to context and that's the limit of what the app needs to know about. In my original suggestion it could be addWindowChannelListener.

Then the whole channel selectors are handled behind the scenes in that method.

So if the window is set to Blue, the listener registered in addWindowChannelListener would receive contexts on the blue channel.
If the window gets set to Pink, then that listener would start receiving context from the pink channel without the app having to do anything.
If the window gets set to Off, then the code within addWindowChannelListener would understand that and not notify the app of any updates until such time that the window is put back upon a channel.

All of this can be managed by global objects placed upon the window object by the container.

@nkolba nkolba added the 1.1 label Dec 6, 2019
@nkolba
Copy link
Contributor

nkolba commented Dec 6, 2019

@chedzoir @pjbroadbent @rikoe @vmehtafds
There's a lot here, so trying to summarize ahead of the next meeting.
I think there are two main topics in this thread (one smaller and one much larger).

Small topic:
Clarifying the "default" channel?
This is what the spec currently says:
The 'system' channels include a 'default' channel which serves as the backwards compatible layer with the 'send/broadcast context' above.
IMHO this means that the "default" really means no channel at all. Which is why it is not behind a getter. I think we should reaffirm that this is what we mean for the default and if so, make the spec more explicit on this point.

larger topic
Should the standard api include filtering context and intent routing via context attributes? I completely agree with the need for the functionality, my central concern on this topic is the standard becoming overly prescriptive. We should map out the various options here. Some questions for the group:

  • is filtering by "type" enough, or will we want to support filtering by identifiers and/or other properties as well?
  • we support metadata in app directory today to filter intents by context type, should app directory also support this for context broadcast?
  • If we filter only by context type, then apps still need to be intelligent enough to know what identifiers they can use and pick these off of the context object (and possibly do their own resolution). If so, then what have we gained by having the desktop agent filter routing by context type?
  • if we have getCurrentContext filter by context type, do we create issues with apps having different "truths" about what the current context is?

@thorsent
Copy link

thorsent commented Dec 13, 2019

In regard to the larger topic. I would upvote a filter for type(s).

  • It provides just a little lift for the app maker. The less code they need to write the better the adoption rate, even if that code is trivial. Personally, I think subscribing to streams of interest by type/topic is likely going to be a frequent ask.
  • Currently, context moves at human speed but I can foresee a future where context begins to move at machine speed. Filtering provides an opportunity for performance optimization.
  • We also could see very large context objects emerging, particularly if applications start "piling on". A filter provides a way to mitigate this potentiality.
  • Most importantly to us, a type filter provides a very useful hint to the resolver. Nick, you'll recall "emitters" and "receptors" from early conversations. Adding the type filter essentially allows an app to behave as a receptor, publishing that it accepts a specific signature. This empowers resolvers to do all sorts of potentially interesting stuff.

Personally, I feel filtering on type is enough but wouldn't argue against a more generalized filtering template if others felt that was useful. We should certainly specify the signature in a way that doesn't preclude adding more complex filtering down the road.

@rikoe
Copy link
Contributor

rikoe commented Jan 22, 2020

@thorsent @nkolba @chedzoir what I would love to see is something like the following (based on Nick's new simplified API):

const myChannel = fdc3.getOrCreateChannel("myChannel");

const listener = myChannel.addContextListener("fdc3.instrument", instrument => {
  // do something with instrument
  // it can be strongly typed because we know its shape definitively
});

The key point for me is that this type of filtering allows:

  1. handlers that know upfront what the shape of the context will be without having to do further pattern matching
  2. the possibility to filter delivery of context data types only to interested parties

Without being able to filter by type, the code would look like this:

const listener = myChannel.addContextListener(context => {
  if (context.type === "fdc3.instrument") {
    const instrument = <Instrument>context;
    // do something with instrument
  } else if (context.type === "fdc3.contact") {
    const contact = <Contact>context;
    // do something with contact
  }
});

Imagine having to do this countless times.

The above is also why I am not really in favour of an array of context types, as you would have something very similar, i.e. no definitive shape of context data:

const listener = myChannel.addContextListener(
  ["fdc.instrument", "fdc3.contact"],
  context => {
    if (context.type === "fdc3.instrument") {
      const instrument = <Instrument>context;
      // do something with instrument
    } else if (context.type === "fdc3.contact") {
      const contact = <Contact>context;
      // do something with contact
    }
  }
);

For this use case, I much prefer:

const instrumentListener = myChannel.addContextListener("fdc3.instrument", instrument => {
  // do something with instrument
});

const contactListener = myChannel.addContextListener("fdc3.contact", contact => {
  // do something with contact
});

This achieves the same result as the array use case, while allowing the code to still be strongly typed, and at the same time it is much easier code to read and write, which is always a good thing.

@rikoe
Copy link
Contributor

rikoe commented Jan 22, 2020

For the record, I think FDC3 should only allow filtering of context by the type property in listeners.

The reason for this is that type is the only mandatory part of any FDC3 context, and is thus the maximum that can be assumed about contexts.

I think filtering by type allows the needed flexibility, if any further filtering is required (e.g. by delving into the id bag, this belongs inside the handler), e.g.:

const listener = myChannel.addContextListener("fdc3.instrument", instrument => {
  if (instrument.ticker) {
     // display ticker
  }
});

@rikoe
Copy link
Contributor

rikoe commented Jan 22, 2020

@nkolba

if we have getCurrentContext filter by context type, do we create issues with apps having different "truths" about what the current context is?

I don't see why this should be the case at all. The implication of filtering by type is simply that there is one truth for each context type, e.g.:

const instrument = await myChannel.getCurrentValue(`fdc3.instrument`);

const contact = await myChannel.getCurrentValue(`fdc3.contact`);`

I really like the symmetry of this, both with the concept of context data with a unique type as the binding principle of FDC3, and with the type-specific addContextListener operation as shown above.

The implication is that instead of maintaining the latest value, the desktop agent has to maintain a map of latest values for each context type. This is a straight-forward expansion of the behaviour.

@chedzoir
Copy link
Author

To add to @rikoe's comment. A lot of this probably boils down to how we consider the current context. Is the current context just one context item or is it a combination? I've seen uses cases where people require the ability to have different context types active as current context.

Do these live on different channels? So you need to listen to multiple channels, at which point this is moot.
Or can they exist on the same channel? If they do then you need to have this ability in place.

It may be that different people expect the channels to work differently, and as a group we ought to be clear what is the expectation and then from that it'd fall out.

@rikoe
Copy link
Contributor

rikoe commented Jan 28, 2020

@chedzoir have we addressed everything with the latest changes? See https://fdc3.finos.org/docs/next/api/Channel and https://github.com/finos/FDC3/blob/master/src/api/interface.ts.

If so, can we close this issue?

@nkolba
Copy link
Contributor

nkolba commented Feb 27, 2020

Addressed with latest context filtering additions.

@nkolba nkolba closed this as completed Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api FDC3 API Working Group
Projects
None yet
Development

No branches or pull requests

6 participants