(WIP) implement addIntentListenerWithContext #1590
Conversation
❌ Deploy Preview for fdc3 failed. Why did it fail? →
|
|
I think that this is all the implementation that is required - pretty minimal actually. Before proceeding to tests and documentation I wanted to get an idea that this is the correct approach. Does this look good so far? Once I get some clarification that this looks somewhat correct I'll start on the documentation and unit tests. Will conformance tests and cucumber tests be required in this PR as well? |
|
I guess this should probably be merged into a 2.3 specific branch? |
There was a problem hiding this comment.
Looks like a good start, a few things to think about:
- How does the DA find out about the contexts? See comment inline
- The reference implementation will probably need updating to actually handle dynamic registration: https://github.com/Roaders/FDC3/blob/00cfc698f03132316cfe2e08a1c6a345dd399e44/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts#L242
and then to use it in each of the functions that resolve intents (findIntent, raiseIntent etc.). - Yes it does need a conformance test adding in https://github.com/Roaders/FDC3/blob/addIntentWithContextListener/website/docs/api/conformance/Intents-Tests.md (with a 2.3 tag on it)
- @robmoffat has a PR open to add the conformance framework into the mono repo, so we will be able to add the test implementation in the same PR soon - but we haven't required that in the past.
| @@ -351,6 +351,66 @@ export interface DesktopAgent { | |||
| */ | |||
| addIntentListener(intent: Intent, handler: IntentHandler): Promise<Listener>; | |||
|
|
|||
| /** | |||
| * Adds a listener for incoming intents raised by other applications, via calls to `fdc3.raiseIntent` or `fdc3.raiseIntentForContext. If the application is intended to be launched to resolve raised intents, it SHOULD add its intent listeners as quickly as possible after launch or an error MAY be returned to the caller and the intent and context may not be delivered. The exact timeout used is set by the Desktop Agent implementation, but MUST be at least 15 seconds. | |||
There was a problem hiding this comment.
You could probably shorten this and reduce duplication by referring to addIntentListener() and explaining the difference?
packages/fdc3-agent-proxy/src/listeners/DefaultIntentListener.ts
Outdated
Show resolved
Hide resolved
| private readonly context: string[] | null, | ||
| action: IntentHandler, | ||
| messageExchangeTimeout: number | ||
| ) { | ||
| super( |
There was a problem hiding this comment.
Theres more to think about here: how does the Desktop Agent know what contexts are being listened for (and hence how to filter intent resolution)?
The AbstractListener is what's notifying the Desktop Agent about the new listener and its only being passed the intent ({ intent }). To resolve, add the contexts array on line 23 if not null (I'd omit it otherwise) then update the addIntentListenerRequest schema to support the extra property in the payload. I'd make it optional (don't add to the required array) but don't allow null values (so type is just array).
|
@Roaders you should have push permissions on fdc3 repo now, so you could merge this into branch of the FINOS repo and open a new PR with that branch. That way the CI Tests and Coverage job will actually pass - we need to do some work on the GitHub action to stop it failing on PRs from forks 😞 |
Co-authored-by: Kris West <kris.west@interop.io>
|
Thanks for the responses @kriswest . I had been thinking earlier in the day about the DA updating it's own inte
8000
rnal representation of the App Directory and that completely fell out of my head when I was working on this. As you say I'll have to update the schema for the I will push to a branch in the finos repo and recreate the PR from there. Thanks for the other comments. |
A PR to |
Co-authored-by: Kris West <kris.west@interop.io>
Describe your change
Adds a new function
addIntentWithContextListenerto theDesktopAgentinterfaces and implements it withinDesktopAgentProxyRelated Issue
resolves #1545
Contributor License Agreement
Review Checklist
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?JSDoc comments on interfaces and types should be matched to the main documentation in /docs
Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
npm run build) run and the results checked in?Generated code will be found at
/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.tsBaseContextschema applied viaallOf(as it is in existing types)?titleanddescriptionprovided for all properties defined in the schema?npm run build) run and the results checked in?Generated code will be found at
/src/context/ContextTypes.tsTHIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE FINOS CORPORATE CONTRIBUTOR LICENSE AGREEMENT.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.