-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FDC3 for Web specification #1191
base: main
Are you sure you want to change the base?
Conversation
Draft Agent agnostic fdc3 spec
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@novavi @Roaders @robmoffat I finally got around to adding some admonitions to the docs about CSPs and the injected iframes - could you give them a look what you have a minute and let me know if they sound correct and complete? See: |
from @hughtroeger's review Co-authored-by: Hugh Troeger <[email protected]>
* `false`. Setting this flag to `true` will inhibit that behavior, leaving | ||
* `window.fdc3` unset. | ||
* | ||
* @property {function} failover An optional function that provides a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to make this property a URL or a function. If a URL, it should basically open a window with that URL, acting in essentially the same way as a window proxy function that you'd end up writing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robmoffat that is what we had in the original proposal. I believe you asked us to simplify it and remove the URL option. All it changes is who writes and maintains the code lives that loads the window (or iframe...) and then waits for it to be ready.
If we go with the URL option we'll need a clear description of how it will behave (as going that route the app will not ahve any control over properties of the window) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about offering a util fn or example implementation in the docs to handle it via copy-paste. I'm somewhat agnostic as to what we do at the moment.
I beleive the original proposal was:
failover?: (args: GetAgentParams) => Promise<URL | WindowProxy | DesktopAgent>
but then who passes in instances of the URL class? It would therefore be Promise<string | WindowProxy | DesktopAgent>
and we'd have some error handling to do if the string was not a URL (reject with AgentError.InvalidFailover
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I was thinking:
failover?: string | (args: GetAgentParams) => Promise<WindowProxy | DesktopAgent>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saves a few characters yes. Code working with the string will need to check its a URL, load it up into an iframe and at least wait for the load event to fire before trying to contact it. Thats the same as you need to do for adaptors loaded into an iframe so it could be handled by a util function.
However, we put the current draft to the discussion group for review and the SWG for adoption already. Given this is at the edge of what we're doing it might be better to wait for a subsequent version and focus on releasing what we've done so far.
Outdated - many changes since that review. Requesting new reviews from maintainers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed by @Roaders, so providing approval on behalf of org.
Co-authored-by: Brian Ingenito <[email protected]>
Make window.fdc3 optional
…-fns Deprecating functions in Methods.ts
resolves #896
This PR adds specifications and documentation related to enabling FDC3 for the web. This was based off of the committee's working doc and an initial PR (#1167) with the draft by @thorsent.
This assumes that "@finos/fdc3" will implement getAgent() and communication mechanisms to work with a Desktop Agent running in a different browser window.
The protocols were codified as "Web Connection Protocol (WCP)" for negotiating the handshake between the library and desktop agents, and "Browser Communication Protocol (BCP)" which encompasses the complete "wire protocol". The former is entirely specific to working in a Web Browser, however the latter might be reused in other implementations and hence may change name (e.g. 'The FDC3 Wire Protocol' or similar).
This documentation followed three streams:
Some changes and additions were made from the original working document:
Deep links to docs for review:
getAgent()
for gaining access to an API implementation for web apps: https://deploy-preview-1191--fdc3.netlify.app/docs/next/api/supported-platformsgetAgent()
reference docs for app implementors: https://deploy-preview-1191--fdc3.netlify.app/docs/next/api/ref/GetAgentAgentError
enumeration added for connection issues: https://deploy-preview-1191--fdc3.netlify.app/docs/next/api/ref/Errors#agenterror