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

Proposed API changes #94

Merged
merged 6 commits into from
Aug 9, 2019
Merged

Conversation

tim-dinsdale
Copy link

For discussion, not final

docs/api/api-spec.md Outdated Show resolved Hide resolved
docs/api/api-spec.md Outdated Show resolved Hide resolved
docs/api/api-spec.md Outdated Show resolved Hide resolved
docs/api/api-spec.md Outdated Show resolved Hide resolved
docs/api/api-spec.md Show resolved Hide resolved
* using the top-level FDC3 `broadcast` function, or using the channel-level {@link broadcast} function on this
* object.
*
* NOTE: Only non-default channels are stateful, for the default channel this method will always return `null`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null or undefined? The latter feels more like idiomatic JavaScript to me - someone could explicitly set the value to null.

src/api/channel/channel.ts Outdated Show resolved Hide resolved
@rikoe rikoe added the api FDC3 API Working Group label May 9, 2019
2. The 'private' or 'app' ones, which have a transient identity and need to be revealed
3. The 'global' one which is available to everyone.

Additionally there is a 'default' channel which serves as /dev/null and satisfies a possible requirements that all apps connect to one channel.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify the use-case for the default channel?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fragment of doc is now out of date - there is no 'global', just the 'default' channel which acts as a way of retconning the old FDC3 global context into this scheme. Are you ok with that?


The 'public' channels include a 'default' channel which serves as the backwards compatible layer with the 'send/broadcast context' above. There are some reserved channel names for future use. Currently this is just 'global'.

To find a desktop channel, one calls

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand how apps are going to use this API. Looking forward to some pseudo code demonstrating the three use cases we talked in the meeting.

I would like to know how an app associates itself with a channel or does a DesktopAgent puts an app in a channel (i.e. in the later case app knows nothing about channels). I was imagining some sort of joinChannel() api somewhere.

Also how this separate channel.broadcast() function is going to work. Is this something an app will call? I think the idea was app doesn't even need to know about channels. Will an app just use regular top level fdc3.broadcast() and the DesktopAgent intercept that and will then use the channel.broadcast() to route it accordingly? How are those two separate broadcast() play nicely with each other.

Same thing about addBroadcastListener() vs addContextListener(). What goes in the app vs the desktopAgent?

Hopefully some code around use cases will clear these things.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of an app associating itself with a channel, something in the app will call getDesktopChannels, will find the channel it needs by checking the members of the DesktopChannelVisualIdentity interface, and will squirrel that away, plus the listener it got from subscribing to it.

In terms of the classic fdc3.broadcast() vs channel.broadcast(): the code is suggesting that one of the desktopChannels will be called 'default', and that if you call fdc3.broadcast() you will send to this channel and vice-versa. In other words, that channel is a facade to the existing single global context.

/**
* A user-readable name for this channel, e.g: `"Red"`
*/
name?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

displayName would be slightly more descriptive, as discussed on our recent working group call.

@@ -0,0 +1,157 @@
type ChannelId = string;

declare interface Listener{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should reuse https://fdc3.finos.org/docs/1.0/api/DesktopAgent#listener for this, which is already ratified as part of v1.0 spec.

1. The 'public' or 'desktop' ones, which have a well understood identity. One is called 'default'.
2. The 'private' or 'app' ones, which have a transient identity and need to be revealed

The 'public' channels include a 'default' channel which serves as the backwards compatible layer with the 'send/broadcast context' above. There are some reserved channel names for future use. Currently this is just 'global'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some reserved channel names for future use. Currently this is just 'global'.

For this to be workable, all reserved words need to be declared at the point this API is released, otherwise application developers aren't able to avoid naming collisions.

public readonly type: string;

/**
* Broadcasts the given context on this channel. This is equivilant to joining the channel and then calling the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/equivilant/equivalent

* within the set of channels registered with the service.
*
* In the case of `desktop` channels (see {@link DesktopChannel}), these id's _should_ persist across sessions. The
* channel list is defined by the service, but can be overridden by a desktop owner. If the desktop owner keeps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is introducing a new concept "desktop owner", which isn't defined in our spec:

https://fdc3.finos.org/docs/1.0/api/api-spec

Any new terminology should be clearly defined in the documentation!

Copy link
Contributor

@nkolba nkolba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tim-dinsdale @rikoe - I've made updates based on the WG feedback (and votes). Can you please check the work and if good, merge!

@rikoe
Copy link
Contributor

rikoe commented Aug 9, 2019

@nkolba @tim-dinsdale Next we need to get the website documentation updated for this.

@nkolba nkolba merged commit 9c2c7b5 into finos:master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api FDC3 API Working Group cla-present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants