Async listener and private channel events by kriswest · Pull Request #1305 · finos/FDC3

kriswest · 2024-08-01T23:13:57Z

resolves #1294
resolves #1300

Makes Listener.unsubscribe() async (the return type is changed from void to Promise<void>) for consistency with the rest of the API.
Adds an async addEventListener function to the PrivateChannel API to replace the deprecated, synchronous onAddContextListener, onUnsubscribe and onDisconnect functions and to keep consistency with the DesktopAgent API.
Deprecates PrivateChannel's synchronous onAddContextListener, onUnsubscribe and onDisconnect functions in favour of an async addEventListener function consistent with the one added to DesktopAgent in Add event listener support to the Desktop Agent API #1207
Adds missing Changle log entry for Add event listener support to the Desktop Agent API #1207

See previews at:

…ontextTypes

netlify · 2024-08-01T23:14:14Z

✅ Deploy Preview for fdc3 ready!

Name	Link
🔨 Latest commit	`2ed30e6`
🔍 Latest deploy log	https://app.netlify.com/sites/fdc3/deploys/66d9b813dbd4f100088f0f01
😎 Deploy Preview	https://deploy-preview-1305--fdc3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

kriswest · 2024-08-01T23:18:13Z

@Roaders @robmoffat This PR addresses the async API call issues you both raised. Let me know what you think.

robmoffat · 2024-08-02T08:04:24Z

I like what you did here to unify the event listener stuff. I guess we're committing to this for 2.2 then?
We haven't actually discussed the listener changes in any meetings yet though

kriswest · 2024-08-02T08:27:25Z

no we haven't. that would need to happen at the next meeting, having the pr and docs preview in hand makes it a lot easier to show the change however!

k

Roaders · 2024-08-02T09:53:34Z

This looks good to me. I don't think that there is any easy way to test this in my local code other than building the .tgz myself and manually installing it in my project right?

kriswest · 2024-08-02T10:31:07Z

Correct. Its easy enough todo however, checkout the relevant branch, run npm run pack which will build the project and then pack the tarball for you. Then simply npm install <path to tarball> in the project where you want to use it.

kriswest · 2024-08-02T12:29:28Z

P.S. @Roaders if you try to do that with the FDC3 for Web PR, note that I haven't got around to adding BrowserTypes.to the exports in index.ts yet. It'll probably conflict with existing API exports so we'll probably need to export it as 'BrowserTypes' the same we do with BridgingTypes.

kriswest · 2024-08-20T09:36:50Z

@Roaders @bingenito @hughtroeger I'll put this on the SWG agenda to look at this week. Would love to get some reviews on it.

bingenito · 2024-08-20T10:41:24Z

+ * as the only parameter.
 */
-export type EventHandler = (event: FDC3Event) => void;
+export type EventHandler = (event: FDC3Event | PrivateChannelEvent ) => void;


Is there a good common base type or marker interface recommendation for this in other languages?

Is it basically just a string and object properties?

Ah the dreaded union type again.

The event types are just strings really, we're just using the enums to govern the set as we do with errors - they are all that really differs. The type names are mutually exclusive between the fdc3 interface events and privateChannel interface events, but it felt like they should be different enums.

I haven't included a common anscestor for the two event types (FDC3Event & PrivateChannelEvent), which is I guess what you are after! I can go add that and have both extend it with specific enums for the types. That would mean EventHandler could use that ancestor here instead...

I'll go take a look at this - I suspect typescript enums are going to make the above more difficult than it should be... Might have to switch to string unions...

I can go with a simple event args type with string and object on C# side but it might be fragile with future changes.

Checkout c47b7b2
Hopefully it'll alleviate your issue here by introducing a base ApiEvent type that the others restrict down to specific messages.

kriswest · 2024-08-20T11:17:05Z

Working on an update that remove the enums as then you can have a workable inheritance structure

…nherit from a common ancestor ApiEvent

kriswest · 2024-08-23T11:06:13Z

Updated to keep event type names style consistent (camelCase) as agreed at SWG meeting 22/08/24. Also, consent was received to make these changes once they are passed through the review of maintainers/editors.

kriswest

This did not merge well with with the .NET PR, particularly in the PrivateChannel.md file - where some work is needed from @bingenito to fill in .NET examples.

THere is also a syntax error somewhere in PrivateChannel.md that I can't find, that is preventing the site from compiling...

kriswest · 2024-09-03T11:30:12Z

+  // functions
+  addEventListener(type: PrivateChannelEventTypes  | null, handler: EventHandler): Promise<Listener>;
+  disconnect(): Promise<void>;
+
+  //deprecated functions
  onAddContextListener(handler: (contextType?: string) => void): Listener;
  onUnsubscribe(handler: (contextType?: string) => void): Listener;
  onDisconnect(handler: () => void): Listener;


@bingenito, need to replicate this in the .NET block below

kriswest · 2024-09-03T11:30:42Z

-            channel.broadcast({ type: "price", price});
-        });
-    });
+    const addContextListener = channel.addEventListener("addContextListener",


@bingenito need to replicate switch to new function in .NET version below

kriswest · 2024-09-03T11:30:59Z

-    const unsubscribeListener = channel.onUnsubscribe((contextType) => {
-        feed.stop(symbol);
-    });
+    const unsubscribeListener = channel.addEventListener("unsubscribe",


@bingenito need to replicate switch to new function in .NET version below

kriswest · 2024-09-03T11:31:01Z

-    const disconnectListener = channel.onDisconnect(() => {
-        feed.stop(symbol);
-    });
+    const disconnectListener = channel.addEventListener("disconnect", 


@bingenito need to replicate switch to new function in .NET version below

kriswest · 2024-09-03T11:31:14Z

+            const listener = result.addContextListener("price", (quote) => console.log(quote));
+                //if it's a PrivateChannel
+                if (result.type == "private") {
+                    result.addEventListener("disconnect", () => {


@bingenito need to replicate switch to new function in .NET version below

kriswest · 2024-09-03T11:31:32Z

-```csharp
-IListener OnAddContextListener(Action<string?> handler);
+```
+Not implemented


@bingenito need to function definition here

bingenito · 2024-09-03T17:47:22Z

This did not merge well with with the .NET PR, particularly in the PrivateChannel.md file - where some work is needed from @bingenito to fill in .NET examples.

THere is also a syntax error somewhere in PrivateChannel.md that I can't find, that is preventing the site from compiling...

This was never implemented. The pending work is under #1318 to done once this is merged.

bingenito · 2024-09-04T10:24:03Z

This did not merge well with with the .NET PR, particularly in the PrivateChannel.md file - where some work is needed from @bingenito to fill in .NET examples.

THere is also a syntax error somewhere in PrivateChannel.md that I can't find, that is preventing the site from compiling...

It seems as though it is trying to transpile the markdown rather than build docs.

kriswest · 2024-09-04T10:37:18Z

This did not merge well with with the .NET PR, particularly in the PrivateChannel.md file - where some work is needed from @bingenito to fill in .NET examples.
THere is also a syntax error somewhere in PrivateChannel.md that I can't find, that is preventing the site from compiling...

It seems as though it is trying to transpile the markdown rather than build docs.

Looks like it - but I think theres a syntax error somewhere that is making it think that the code block is markup it needs to process. Its fine with other code blocks on that page so it's likely a stray or missing character I'm just not seeing...

kriswest · 2024-09-04T10:37:45Z

This did not merge well with with the .NET PR, particularly in the PrivateChannel.md file - where some work is needed from @bingenito to fill in .NET examples.
THere is also a syntax error somewhere in PrivateChannel.md that I can't find, that is preventing the site from compiling...

This was never implemented. The pending work is under #1318 to done once this is merged.

SHall I just go and add the tab but put Not implemented yet in it?

bingenito · 2024-09-04T10:40:01Z

Unless you think it is causing this issue I wouldn't bother as I'll do it with a PR after this merge. I think there is a different issue here perhaps with mismatched tags that I'm looking at. The syntax of that example is the same in the merged PR.

bingenito · 2024-09-04T12:08:48Z

That force push was for a git commit amend of my last commit message

kriswest · 2024-09-05T13:53:41Z

@hughtroeger no worries - thanks for getting to another one!

I spotted a syntax error (another stray space char):

Fixed, could you re-approve @hughtroeger ?

kriswest added 3 commits August 1, 2024 22:41

make Listener.unsubscribe() async

1a76a9b

Adjust imports to correct conflict with equivalent type export from C…

439b49d

…ontextTypes

Update PrivateChannel to use the addEventListener pattern

79c503a

kriswest requested review from a team August 1, 2024 23:17

kriswest added enhancement New feature or request api FDC3 API Working Group deprecation Deprecating or removing features post deprecation labels Aug 2, 2024

Changelog

e72eabf

bingenito reviewed Aug 20, 2024

View reviewed changes

Reworking addEventListener to avoid union types in .NET, events now i…

c47b7b2

…nherit from a common ancestor ApiEvent

kriswest requested a review from bingenito August 20, 2024 11:52

bingenito mentioned this pull request Aug 22, 2024

Add .NET docs for Events to API reference #1318

Closed

bingenito reviewed Aug 22, 2024

View reviewed changes

Comment thread src/api/Events.ts Outdated

kriswest added 2 commits August 23, 2024 12:03

Merge branch 'main' into async-listener-and-PrivateChannel-events

2bd62fa

Make event type names style consistent (camelCase)

8654cbe

kriswest requested review from a team and bingenito August 23, 2024 11:06

kriswest assigned bingenito and hughtroeger Aug 23, 2024

kriswest dismissed bingenito’s stale review via 5719b33 September 2, 2024 15:59

kriswest requested review from bingenito and hughtroeger September 2, 2024 16:30

bingenito previously approved these changes Sep 3, 2024

View reviewed changes

Merge branch 'main' into async-listener-and-PrivateChannel-events

313f862

kriswest dismissed bingenito’s stale review via 313f862 September 3, 2024 11:22

kriswest added 3 commits September 3, 2024 12:28

fix syntax error on tabs

9e3bf09

fixing another syntax issue

889852d

more syntax errors - still one present I can't find

32722dc

kriswest commented Sep 3, 2024

View reviewed changes

Remove spaces in OnUnsubscribe csharp markdown

4f027b8

bingenito force-pushed the async-listener-and-PrivateChannel-events branch from aff0c68 to 4f027b8 Compare September 4, 2024 11:16

Remove extra spaces on Disconnect and OnDisconnect

a07e776

bingenito previously approved these changes Sep 4, 2024

View reviewed changes

kriswest requested a review from a team September 4, 2024 13:40

hughtroeger previously approved these changes Sep 4, 2024

View reviewed changes

stray space chars

2ed30e6

kriswest dismissed stale reviews from hughtroeger and bingenito via 2ed30e6 September 5, 2024 13:54

bingenito approved these changes Sep 5, 2024

View reviewed changes

kriswest merged commit c3f52ea into main Sep 5, 2024

kriswest deleted the async-listener-and-PrivateChannel-events branch September 5, 2024 14:30

kriswest added the needs-conformance-test label Oct 24, 2024

Conversation

kriswest commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for fdc3 ready!

Uh oh!

kriswest commented Aug 1, 2024

Uh oh!

robmoffat commented Aug 2, 2024

Uh oh!

kriswest commented Aug 2, 2024

Uh oh!

Roaders commented Aug 2, 2024

Uh oh!

kriswest commented Aug 2, 2024

Uh oh!

kriswest commented Aug 2, 2024

Uh oh!

kriswest commented Aug 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriswest commented Aug 20, 2024

Uh oh!

Uh oh!

kriswest commented Aug 23, 2024

Uh oh!

kriswest left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bingenito commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bingenito commented Sep 4, 2024

Uh oh!

kriswest commented Sep 4, 2024

Uh oh!

kriswest commented Sep 4, 2024

Uh oh!

bingenito commented Sep 4, 2024

Uh oh!

bingenito commented Sep 4, 2024

Uh oh!

kriswest commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kriswest commented Aug 1, 2024 •

edited

Loading

netlify Bot commented Aug 1, 2024 •

edited

Loading

kriswest left a comment •

edited

Loading

bingenito commented Sep 3, 2024 •

edited

Loading