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

1.1 api simple #154

Merged
merged 7 commits into from
Jan 23, 2020
Merged

1.1 api simple #154

merged 7 commits into from
Jan 23, 2020

Conversation

nkolba
Copy link
Contributor

@nkolba nkolba commented Jan 17, 2020

The purpose of this pull request is the following:

  • More fully document the channels apis, adding documentation into the API Reference section
  • Simplifying some aspects of the channels API. Specifically:
    • Removing the Channels namespace, and moving some methods to the top-level fdc3 space
    • Rationalizing system and app channels - so that they are only different by convention
    • Added an error enum for channels, and made it explicit that the desktop agent could reject access to a channel based on the permissions of the app

@nkolba nkolba requested a review from rikoe January 19, 2020 03:42
@nkolba nkolba added 1.1 api FDC3 API Working Group labels Jan 19, 2020
Copy link
Contributor

@rikoe rikoe left a comment

Choose a reason for hiding this comment

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

I really like this simplification. It makes the API a lot simpler to understand and use. Well done!

Added a few minor improvements that can be made.

docs/api/api-spec.md Outdated Show resolved Hide resolved
docs/api/api-spec.md 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
@nkolba
Copy link
Contributor Author

nkolba commented Jan 21, 2020

thanks @rikoe! I've made the recommended changes and pushed to my branch

* `Error` with a string from the `ChannelError` enumeration.
*/
joinChannel(channelId: string) : Promise<void>;

Choose a reason for hiding this comment

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

All other channel interactions happen through the Channel object, and for consistency it feels like this one should too.

Can this method be moved to Channel.join(): Promise<void>?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reason is that if it is at the top level you could just join a channel by id without having to do a “get and search” first, so it massively simplifies the code for someone who just wants to join a channel and then use the existing top-level broadcast or context listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

* Channels may be visualized and selectable by users. DisplayMetaData may be used to provide hints on how to see them.
* For app channels, displayMetadata would typically not be present
*/
displayMetadata?: DisplayMetadata;

Choose a reason for hiding this comment

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

Add public readonly modifiers?

Also, comment refers to "DisplayMetaData", should be "DisplayMetadata".


try {
let myChannel = await fdc3.getOrCreateChannel("myChannel");
myChannel.addContextListener(listener);
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 not correct. Should be: const listener = myChannel.addContextListener(context => { })

@nkolba nkolba merged commit 84bd4ea into finos:master Jan 23, 2020
@nkolba nkolba deleted the 1.1-api-simple branch January 23, 2020 16:42
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.

4 participants