Skip to content
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

Merged
merged 196 commits into from
Nov 13, 2024
Merged

FDC3 for Web specification #1191

merged 196 commits into from
Nov 13, 2024

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented Apr 15, 2024

resolves #896
resolves #1385
resolves #1402

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:

  • Modifications for users of FDC3 - primarily related to using getAgent() for connectivity instead of relying on the global fdc object
  • Instructions for desktop agent implementers - provides a specification for "browser-resident desktop agents" as well as "preload desktop agents", along with typescript definitions.
  • A specification for getAgent() - to be used by implementers of the "@finos/fdc3" library, and for reference by desktop agent implementers.

Some changes and additions were made from the original working document:

  • The optional initialization fields for channel selector and intent resolver were eliminated. The DA knows whether it can provide these UI elements and so therefore it should be the DA that decides how to accomplish this. However, there are cases where a DA cannot reasonably provide these UI elements in a browser, therefore this spec mandates that "@finos/fdc3" provide built-in UI capabilities that a DA can optionally enable them.
  • This spec also covers window disconnects by suggesting use of a well known polling mechanism.
  • A new folder called "specs" was created to house many of the specifications involved. Previously, there was no true distinction between implementation specs and user specs in the FDC3 overview.

Deep links to docs for review:

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for fdc3 canceled.

Name Link
🔨 Latest commit ed22949
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/6734b99fd9905a0008cbe86f

@kriswest kriswest marked this pull request as draft April 17, 2024 09:47
@robmoffat

This comment was marked as outdated.

@kriswest

This comment was marked as outdated.

@kriswest
Copy link
Contributor Author

kriswest commented Nov 4, 2024

@kemerava many thanks for the detailed review. I've resolved most of the comments for things fixed. I've left a few open for you to look at, please resolve those as well. Our branch protection will require you to post another review/confirm requested changes have been made.


```js
const desktopAgent = await getAgent({
timeoutMs: 250,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timeouts are usually counted in milliseconds. Isn’t it overkill to name it timeoutMs rather than just timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Pavlo, we actually started out with timeout but moved to timeoutMs after review comments suggested its slightly more intuitive. I think it probably boils down to personal preference.

@@ -603,7 +613,7 @@ To find a User channel, one calls:
```ts
// returns an array of channels
const allChannels = await fdc3.getUserChannels();
const redChannel = allChannels.find(c => c.id === 'red');
const redChannel = allChannels.find(c => c.id === "red");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be the correct example.

const redChannel = allChannels.find(c => c.id === "fdc3.channel.1");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point (match IDs to the recommended FDC3 channels). I'll implement that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, well technically this example significantly predates this PR (and in fact the recommended channels).

To be honest, I don't think it a good example at all. If you already know the channel name (as it the case in this example) you wouldn't do this at all. Rather you would more simply do:

const redChannel = fdc3.getOrCreateChannel("fdc3.channel.1");

Frankly, I think we should replace this example and the matching one at: https://fdc3.finos.org/docs/api/ref/DesktopAgent#getuserchannels
Particularly as the use case for fdc3.getUserChannels is not to find a specific channel, but to get the whole list to do something with.

Let me know if you're up for raising that as a separate issue (and perhaps PR), otherwise I can take care of it. It can be done quickly and without a vote as its not changing the standard, jsut improving its docs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this documentation improvement can be done in-place as it doesn't affect any functionality.

@robmoffat robmoffat requested review from mistryvinay and removed request for novavi November 7, 2024 14:04
@kriswest kriswest requested review from kemerava and novavi November 7, 2024 14:05
robmoffat
robmoffat previously approved these changes Nov 8, 2024
@@ -2,7 +2,7 @@
"$schema": "http://json-schema.org/draft-07/schema#",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think we're going to need this message. Would be nice to merge my PoC into @julianna-ciq 's add-on module to prove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach that you very briefly explained to positioning the iframe from inside by first resizing it to cover the whole parent window still seems risky, likely to cause display artefacts and is likely slower to apply (requires more messaging and rendering).

We've also reviewed the current standard docs multiple times and checked the consensus of the SWG on them which we'd have to repeat if changing them significantly. We would need both a write-up and discussion of what you propose doing to change it and, personally, I don't think it worth further delaying this project to do so - no actual benefit to it has been articulated as yet.

hughtroeger
hughtroeger previously approved these changes Nov 11, 2024
Copy link
Contributor

@hughtroeger hughtroeger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, content looks good 👍

docs/api/ref/GetAgent.md Outdated Show resolved Hide resolved
schemas/api/api.schema.json Outdated Show resolved Hide resolved
schemas/api/getInfoRequest.schema.json Outdated Show resolved Hide resolved
Copy link
Contributor

@openfin-johans openfin-johans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@kriswest kriswest merged commit 5b9c140 into main Nov 13, 2024
7 checks passed
@kriswest kriswest deleted the fdc3-for-web branch November 13, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet