Add intent with context listener #1594
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
sorry about the whitespace changes on here. Do we have anything in place to lint json changes to prevent this sort of change?
There was a problem hiding this comment.
Apparently not. The schema files should probably be being processed by prettier to do that. For some reason JSON files are being ignored:
Lines 1 to 6 in 63eceaa
Feel free to change that and see what it does to the schema files - although I'd suggest doing so in a different PR
There was a problem hiding this comment.
This is unfortunately not as easy as it would seem. Eslint does not support json files. I think that I am correct in saying that prettier is run via ESlint in this repo. Unfortunately I don't think that we can get prettier to handle json files when run via eslint.
In the past I have had to add a prettier call to my lint command to do this:
"lint": "prettier -c **/*.json && eslint"
There was a problem hiding this comment.
No I think its lint-staged running prettier on all projects and doing so on commit:
Lines 65 to 67 in d4a261a
https://github.com/lint-staged/lint-staged
The eslint integration with prettier is to adjust the eslint rules (which differ from prettier by default on some areas of formatting). More details here: https://prettier.io/docs/integrating-with-linters
In short, adjusting the .prettierignore should apply it to our JSON files - although it will only apply on commit to ones you touched I think. To manually run it on everything to see what. changes, try npx prettier . --write in teh root dir.
|
|
57da80a to
716d00f
Compare
|
I think this covers most of the functionality but as far as I can see there is still a gap. In the reference implementation I cannot see how an app that registers an intent with @robmoffat - can you give me some guidance on how this works? |
|
|
|
Converted to draft as docs, unit tests and conformance tests still need to be updated |
|
Hi @Roaders, To answer your question above, take a look at when someone adds an intent listener. The directory isn't changed itself - this is static based on what is loaded from the AppD endpoints. You're going to need to add some tests (see HTH? |
|
|
Yup, I have updated the Can you point to the code that gets the list of application to display in the resolution dialog? Thanks |
Yeah I can't see it either. Seems like it only looks at what the appD record declares. I'm not sure if this is correct behaviour or not, but It passes the conformance tests so if it isn't we are probably missing some tests. My feeling is this might be a gap in the spec: the directory |
|
I thought about a conformance test but it would have to be a manual one as the user would have to select the registered app in the app resolver for the test to pass... |
|
FWIW it does use registrations in findIntents. |
|
@Roaders @robmoffat its upto the desktop agent to return the options for further resolution. Thats handled here in the proxy: FDC3/packages/fdc3-agent-proxy/src/intents/DefaultIntentSupport.ts Lines 159 to 165 in 63eceaa The DA has sent back app intents for all resolution options, which are then passed to the (injected) intent resolver. If the DA showed its won resolver then it doesn't return the appIntent as its already resolved, so this path is only for injected resolvers. In the reference implementation intent resolution happens in this block (depending on whether and how a target was set): FDC3/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts Lines 455 to 474 in 63eceaa The reference implementation doesn't currently support dynamic registration AFAIK (and should be updated to do so) as the resolution always relates to the app directory, that happens here : FDC3/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts Lines 384 to 385 in 63eceaa This block is where it will resolve if no target was set and if it only found one option: FDC3/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts Lines 407 to 423 in 63eceaa Then finally here it returns options to show in the resolver if multiple options were found: FDC3/toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts Lines 424 to 439 in 63eceaa As you guys are finding this hard to navigate perhaps we need to add some more comments to it? It does take a while to read and interpret that code... |
A manual test is worth adding. However, I did just have a random thought about injecting a resolver just for testing that can do the selection... but then not all DAs will use injection for the resolver so that might be too specific. |
Dynamic registration is currently optional (MAY) so we don't test for conformance on it.
We have a specified error ( Line 536 in 63eceaa
...and thats optional support for dynamic registration. Its all covered in the spec (no gaps) but its also not entirely standardized as dynamic registration is optional (MAY). Does that answer all questions raised in this thread? |
|
Thanks very much for the detailed (as always) replies @kriswest and @robmoffat . Not everything is clear to me yet though I am afraid. To summarise what's been said so far:
@kriswest I am a bit confused regarding adding a conformace test. You replied to my comment saying that we should add a conformance test to ensure that if an intent lisstener is registered that it appears in the resolution dialog but you also replied to Rob to say that dynamic registration is optional so we don't need a conformace test. It seems to me that we don't need a conformance test as this is an optional feature. I will add to the |
We do not desperately need a test if its not a required feature - although there is an argument that if an optional feature (MAY OR SHOULD) is present it MUST be conformant to the spec... However, I made the comment in the context of this, perhaps, becoming a required feature. If we accept the addition of the new API call it should be a MUST (we don't have any other optional API calls AFAIK). Then we should consider whether dynamic registration should be required of a Desktop Agent or not. If it's NOT supported, then the standard is somewhat deficient on advice as to how to handle that (so I misspoke above - that is a gap!). There's no suitable error to return so most such implementations probably just accept the registration but never route anything to it/offer it during resolution. Is that the best solution? If so we should document that. Hence, I think we have something that should be added to the spec whether we require dynamic registration or not.
💯 |
|
Raised #1610 |
|
The reference implementation now seems to correctly add dynamically registered apps to the app resolver. |
This one! Sorry if that wasn't clear! |
|
I was always expecting to do tha twork as part of this PR. Should I not be? |
|
Hi @Roaders, I think this might be crossing the streams a bit: that last part is a bug-fix that we should really have in the 2.2 branch, whereas the rest of this PR is for FDC3 2.3, I think |
Yeah, that makes sense. I can cherry pick and raise a PR with just that fix - unless you've already done it? |
|
ok, raised #1613 as a seperate PR. |
454cba7 to
7095ee7
Compare
Co-authored-by: Kris West <kris.west@interop.io>
7095ee7 to
bae31a6
Compare
|
|
|
remaining items that need looking at:
|
Describe your change
Adds a new function
addIntentWithContextListenerto theDesktopAgentinterfaces and implements it withinDesktopAgentProxyRelated Issue
resolves #1545
recreating #1590 - to be closed
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.