Async listener and private channel events #1305
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@Roaders @robmoffat This PR addresses the async API call issues you both raised. Let me know what you think. |
|
I like what you did here to unify the event listener stuff. I guess we're committing to this for 2.2 then? |
|
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 |
|
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 |
|
Correct. Its easy enough todo however, checkout the relevant branch, run |
kriswest
added
enhancement
|
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. |
|
@Roaders @bingenito @hughtroeger I'll put this on the SWG agenda to look at this week. Would love to get some reviews on it. |
src/api/Events.ts
Outdated
| */ | ||
| export type EventHandler = (event: FDC3Event) => void; | ||
| export type EventHandler = (event: FDC3Event | PrivateChannelEvent ) => void; |
There was a problem hiding this comment.
Is there a good common base type or marker interface recommendation for this in other languages?
There was a problem hiding this comment.
Is it basically just a string and object properties?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
I can go with a simple event args type with string and object on C# side but it might be fragile with future changes.
There was a problem hiding this comment.
Checkout c47b7b2
Hopefully it'll alleviate your issue here by introducing a base ApiEvent type that the others restrict down to specific messages.
|
Working on an update that remove the enums as then you can have a workable inheritance structure |
…nherit from a common ancestor ApiEvent
|
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. |
There was a problem hiding this comment.
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...
| // 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; |
There was a problem hiding this comment.
@bingenito, need to replicate this in the .NET block below
| channel.broadcast({ type: "price", price}); | ||
| }); | ||
| }); | ||
| const addContextListener = channel.addEventListener("addContextListener", |
There was a problem hiding this comment.
@bingenito need to replicate switch to new function in .NET version below
| const unsubscribeListener = channel.onUnsubscribe((contextType) => { | ||
| feed.stop(symbol); | ||
| }); | ||
| const unsubscribeListener = channel.addEventListener("unsubscribe", |
There was a problem hiding this comment.
@bingenito need to replicate switch to new function in .NET version below
| const disconnectListener = channel.onDisconnect(() => { | ||
| feed.stop(symbol); | ||
| }); | ||
| const disconnectListener = channel.addEventListener("disconnect", |
There was a problem hiding this comment.
@bingenito need to replicate switch to new function in .NET version below
| const listener = result.addContextListener("price", (quote) => console.log(quote)); | ||
| //if it's a PrivateChannel | ||
| if (result.type == "private") { | ||
| result.addEventListener("disconnect", () => { |
There was a problem hiding this comment.
@bingenito need to replicate switch to new function in .NET version below
| ```csharp | ||
| IListener OnAddContextListener(Action<string?> handler); | ||
| ``` | ||
| Not implemented |
There was a problem hiding this comment.
@bingenito need to function definition here
This was never implemented. The pending work is under #1318 to done once this is merged. |
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... |
SHall I just go and add the tab but put Not implemented yet in it? |
|
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
force-pushed
the
async-listener-and-PrivateChannel-events
branch
from
aff0c68
to
4f027b8
Compare
|
That force push was for a git commit amend of my last commit message |
|
@hughtroeger no worries - thanks for getting to another one! I spotted a syntax error (another stray space char): Fixed, could you re-approve @hughtroeger ? |
resolves #1294
resolves #1300
Listener.unsubscribe()async (the return type is changed fromvoidtoPromise<void>) for consistency with the rest of the API.asyncaddEventListenerfunction to thePrivateChannelAPI to replace the deprecated, synchronousonAddContextListener,onUnsubscribeandonDisconnectfunctions and to keep consistency with the DesktopAgent API.PrivateChannel's synchronousonAddContextListener,onUnsubscribeandonDisconnectfunctions in favour of anasyncaddEventListenerfunction consistent with the one added toDesktopAgentin Add event listener support to the Desktop Agent API #1207See previews at: