FDC3
FDC3 copied to clipboard
The `AppIntent` interface incorrectly describes an intent
Minor Issue
The AppIntent
interface incorrectly describes an intent. The displayName
of the AppIntent
's intent
is ambiguous.
Area of Issue
[ ] App Directory [X] API [ ] Context Data [X] Intents [ ] Use Cases [ ] Other
Issue Description:
Let's say we have the following FDC3 application definitions (this is just an example to illustrate the issue - some required properties are missing):
[
{
"name": "ChartA",
"intents": [
{
"name": "fdc3.ViewChart",
"displayName": "Chart A's View Chart Intent",
"contexts": [
"fdc3.instrument"
]
}
]
},
{
"name": "ChartB",
"intents": [
{
"name": "fdc3.ViewChart",
"displayName": "Chart B's View Chart Intent",
"contexts": [
"fdc3.instrument"
]
}
]
}
]
After calling fdc3.findIntent('fdc3.ViewChart')
we would get the following AppIntent
:
{
intent: {
name: 'fdc3.ViewChart',
displayName: '...'
},
apps: [
{
name: 'ChartA'
},
{
name: 'ChartB'
}
]
}
The issue is that the display name is ambiguous - it could be Chart A's View Chart Intent
or Chart B's View Chart Intent
.
Going through the existing open-source FDC3 implementations we noticed that both Nick Kolba's desktop-agent, as well as Finsemble's FDC3 implementation would return an AppIntent
with the intent display name of the first application.
Additional Context:
A PR for this issue would require a change to the existing AppIntent
interface.
This is a good catch, we need to fix this, and 2.0 will be a good place to do this, as it may require breaking changes to the AppIntent
interface.
Should we just drop the displayName field from IntentMetadata (which leaves it as a rather pointless class containing only a single string name
) ? After all intents should be:
- Recognizable
- Generally self-evident what the thing is according to the intents spec.
We could then also drop displayName
from the appD intents object.
An alternative would be to add some form of reference for intents (other than the config of individual apps), perhaps as part of the appD, where the display name could be drawn from.
@ggeorgievx any thoughts on how to resolve this issue? I can see how this flaw might have come about:
- There exists suggested display names for the standard intents: https://github.com/finos/FDC3/blob/master/src/intents/standard%20intents.json
- However, the spec/docs don't point to this AND there's no such reference for a custom intent, apart from what an individual app names it in their intent config.
When Citi showed us their AppD portal, it included a reference for both intents and context types. Perhaps we should go that route, add it as a new record type to the appD and remove the displayName ?config?? Perhaps with a suggested fail-over to the raw intent name?
IMO the cheapest solution would have us add an optional displayName
property to the AppMetadata
interface similarly to how we dealt with application instances in #509.
Another solution would be to change the AppIntent
interface's apps
property to a singular from (app
). That way we would have the AppIntent
interface describe a single instance/implementation of an intent with the correct metadata like
the displayName
(and would truly be an AppIntent
as opposed to an AppsIntent
). We would then also need to change the signature of findIntent()
(and possibly the name to findIntent**s**()
) to return a promise resolving with an array of AppIntent
(Promise<AppIntent[]>
).
Thoughts?
I personally like this suggestion @ggeorgievx - I think it is the "best" way to solve the problem, and the most flexible for the future. An AppIntent represents the relationship between a single app and intent, and you could have multiple of them returned...
@kriswest @mattjamieson Team, what are your thoughts on this one? If you agree with Riko I am more than happy to raise a PR.
Alternatively we could have this added to the mtg agenda for the next Standards WG.
I agree that this change makes sense. The semantics of AppIntent have always bothered me as has the hack you have to use to pick a display name for intents.
The affected functions are just these two, right?
// intents
findIntent(intent: string, context?: Context): Promise<AppIntent>;
findIntentsByContext(context: Context): Promise<Array<AppIntent>>;
We could/should switch both to return Promise<Array<AppIntent>>
, with each entry then corresponding to single app - there can be a mix of intents in the array in the findIntentsByContext
response, but I'd be inclined to be consistent over the return type rather than goto a 2d array (particularly as every entry in the response to findIntent
could have a different displayName
even when the intents are all the same).
The (breaking) change will of course need approval by the SWG, but I think it's worth implementing the PR to present to the group. Don't forget that it will also need fixing in the Methods.ts.
@thorsent are you also happy with this change?
P.S. apologies for the delay in responding @ggeorgievx, I missed the original notification.
One more thing to consider... Rex suggested this in the deprecation policy (see #532):
Rather than quietly change the behavior of an existing feature, consider deprecating it and replacing it with something with a different name/syntax.
This is a breaking change without a prior deprecation. Should we consider revising the function and type names and preserve the old ones as deprecated? Implementing a fallback/conversion in Methods.ts is possible.
Not sure what I would call the replacement for AppIntent
... IntentApp
? Implies findIntentApp
and findIntentAppByContext
@kriswest I like the change.
I think it's worth discussing how to deal with breaking changes more elegantly. Perhaps namespacing offers a more discrete approach:
//Maintain separate namespaces for each official version
export namespace FDC3_1 {
export type FDC3 = {
findIntentsByContext(context: string): Promise<AppIntent>
}
}
export namespace FDC3_2 {
export type FDC3 = {
findIntentsByContext(context: string): Promise<Array<AppIntent>>
}
}
// Default to the most recent version
export type FDC3 = FDC3_2.FDC3 & {
fdc3_1 ?: FDC3_1.FDC3, // But provide access to prior versions - if the desktop agent supports it.
fdc3_2 ?: FDC3_2.FDC3
}
Since fdc3
is a declared global though, this would force developers to be a little more creative:
myapp.ts
import { FDC3_1 } from "@types/@finos/fdc3";
let myFDC3 : FDC3_1 = fdc3.fdc3_1;
const appIntent = await myFDC3.findIntentsByContext("chart");
@thorsent @ggeorgievx @rikoe For the System channels -> User channels rename, where functions changed name (e.g. getSystemChannels()
-> getUserChannels()
) we implemented an automated fallback in the NPM Module's Methods.ts. There's another for the addContextListener signature that was deprecated here.
Not everyone uses that NPM module, but it does provide an alternative to namespacing that we could use... It does work best if we change the name of the function and types when making breaking changes (as its easy to detect that the new function is not present and we need to adapt).
Finally, my assumption is that it will usually be easier to update the NPM module in an application than it will be to update the desktop container (which might only provide FDC3 1.2, where your app has been updated to use FDC3 2.0).
The fallback works fine for JS (and desktop agents could also offer backward compatibility).
The only issue is that existing TS implementations would break during the compile cycle when the published types change. So I think it depends on whether that is an acceptable compromise.
On returning to this issue, I am less sure about the proposed change (allowing for a display name per intent/app combo). There are situations (e.g. a button to raise an intent in an app, which can then be resolved by multiple other apps) and API calls (findIntentsByContext
and raiseIntentForContext
and resolver responses/UI) where we need a single display name for an intent. There is a use-case for data unique to an intent/app combo, but its perhaps descriptive rather than nominative (what precisely will the app do when it receive the roughly descriptive intent).
The /intents
appD endpoint proposed in #719 also doesn't provide a solution where multiple appDs are in use as you could still end up with multiple display names per intent. We could apply the display names in the standard itself; there exists a source file with display names although I think these are rarely used and not necessarily correct, e.g.:
{
"name": "ViewInstrument",
"displayName": "Details"
},
{
"name": "ViewAnalysis",
"displayName": "Analyze"
},
{
"name": "ViewHoldings",
"displayName": "Holdings"
}
It also doesn't provide a solution for proprietary or as yet unstandardized intents.
Hence, I am again leaning towards simply dropping the display name, in favour of just using the intent name (and living with PascalCase) as that provides a simple solution for all cases.
Notes from #802:
- The AppIntent interface only allows for a single display name to be returned. However, the main source for the display name is the interop/intents configuration of apps that can resolve the intent - hence, multiple different display names maybe returned, forcing the desktop agent to pick one.
- This was less of a problem in FDC3's early days when a Desktop Agent would typically be working with a single appD in the control of the desktop developer, allowing them to ensure consistency in the names.
- However, in situations where the Desktop Agent develop, (multiple) app developers and the desktop owner all differ it becomes far more challenging.
- Proposed solutions:
- Alter the
AppIntent
type to allow a display name per app returned- Challenging for a resolver UI or applications using
findIntent
as they are likely to need a single display name - Also a breaking change...
- Challenging for a resolver UI or applications using
- Advise that display names are drawn from the 'standard intents.json' file in the standard
- The display names in that file have not been well curated and vary in form
- Does not provide a solution for proprietary intents.
- Deprecate the
displayName
field and rely on the intents name instead.- Intents are already required to be 'recognizable' and their meaning to be 'generally self-evident'
- Non-breaking (until field is removed in a later version)
- Alter the
- However, there is a use-case for some descriptive information about a specific apps handling of an intent: describe the precise behavior of the application.
- This could be handled through by allowing an app to configure a description field in its
interop.intents.listensFor
metadata (breaking change) and a type that extendsAppMetadata
in the Desktop Agent API to add the description field (non-breaking). - A breaking change after the 2.0 release is undesirable.
- This could be handled through by allowing an app to configure a description field in its
- PR for the change (deprecate display name, explain in documentation that intent name should be used for display) to be worked up and brought back for further review