Clarification re repeated addContextListener and addIntentListener calls #1394
Conversation
❌ Deploy Preview for fdc3 failed. Why did it fail? →
|
There was a problem hiding this comment.
Good job! Personally, I think that we triggering of all listeners is currently a MUST and should remain so (meaning the SHOULDs in the this PR become MUSTs to explicitly state that).
I also wonder if we need to add a comment about reconnection. If a web app is refreshed it needs to add new handlers as the old ones will be fone - the DA should be tracking the lifecycle of the app and considering them to have been removed if the app reloads/navigates/reconnects. That generally only applies to web apps as most others don't 'reconnect' or 'refresh' in a way that would wipe out their handler functions
docs/api/ref/Channel.md
Outdated
| @@ -174,6 +174,8 @@ If, when this function is called, the channel already contains context that woul | |||
|
|
|||
| Optional metadata about each context message received, including the app that originated the message, SHOULD be provided by the desktop agent implementation. | |||
|
|
|||
| Adding multiple context listeners on the same or overlapping types (i.e. specific `contextType` and `null` type) SHOULD be allowed, and SHOULD trigger all ContextHandlers when a relevant context type is broadcast on the current channel. | |||
There was a problem hiding this comment.
We should probably upgrade this to a MUST or it allows variation between implementations that may be undesirable. There is no error to return as an alternative so an app will have no way to know if a new listener was added or an existing one replaced (until they can observe it not receiving something it should).
My preference would be to have consistent behavior and fos us to add a conformance test for it in both:
There was a problem hiding this comment.
Changed the SHOULD to MUST
There was a problem hiding this comment.
I also added the test definitions to the files you mentioned; do you think it is also worth adding the addIntentListener case to the Basic tests?
|
I see a mix of resolvererror and resolveerror in text and anchor links. |
Good 👀 @bingenito - I missed several of those. Added to suggestions |
Co-authored-by: Kris West <kris.west@interop.io>
@kriswest In terms of the reconnection, I think that there a case which can to be made for Desktop Agent to keep the listeners in memory at least for a short period of time. Not sure as much for web apps, but for others, depending on which method of connection is used, it is possible that the app temporarily lost the connection, and I am not sure if it makes sense to always ask apps to reconnect. |
Closes #1390
Based on the discussion on the issue, adding the clarification to the standard on the expected behavior when multiple calls are made to addContextListener and addIntentListener
@michael-bowen-sc