feat: add fdc3Ready helper function and getInfo to npm package exports

[rikoe]

To add support to the FDC3 NPM package for the new fdc3Ready event, this adds an asynchronous fdc3Ready() function that has the following properties:

• it will resolve immediately if window.fdc3 is already defined
• it has a default timeout of 5 seconds (as recommend by @kriswest), but can be called with a smaller wait time as well, e.g. await fdc3Ready(1000)
• after the timeout expires, or the fdc3Ready event is fired, it will again check if window.fdc3 is defined, and resolve/reject as appropriate

Also adds the getInfo method as an exported function, which was missing.

Tests for all of the above are included, as is documentation on the FDC3 website.

Example usage:

import { fdc3Ready, broadcast } from '@finos/fdc3'

try {
  await fdc3Ready(1000) // wait 1 second before timing out
  fdc3.broadcast({ ... })
} catch (error) {
  // handle
}

[thorsent]

I think that there's not really much point in trying to auto-magically check fdc3Ready because the API isn't entirely async (promisified). broadcast() and addContextListener() are by far the most likely calls that a developer will make in the global scope, and those will both throw throwIfNoGlobal(), forcing the developer to manually implement the clumsy fdc3Ready logic, and negating the value of the auto-magic.

I see two possibilities to remedy this:

One is to simply require that desktop agents provide a global fdc3 and handle it all. (e.g. Cosaic's fdc3 implementation has implemented simple queuing logic so that developers don't need to worry about fdc3Ready.)

The second possibility is to promisify the remaining calls (e.g. broadcast(), addContextListener(), addIntentListener()) so that the hidden await pattern can be implemented across the board.

[rikoe]

Thanks @thorsent.

I do agree that making the three synchronous calls is counter-productive, I think in a future release, everything should be promisified. Doing this work has made clear to me how problematic those three functions that are synchronous are.

I basically see two options:

1. Just keep the current code with rejects or throws if fdc3 isn't available, and recommend users to manually call fdc3Ready() function, once only (which has the added benefit of not running code which isn't necessary).

   The function still makes it slightly easier to handle the window event listener pattern, by wrapping it as a promise with a timeout.

2. Automatically call fdc3Ready() in all imported functions for safety of users who just do import { raiseIntent } from '@finos/fdc3' without understanding the nuances of the global object or ready event.

   This is with the understanding that it won't be possible for synchronous operations like broadcast until we promisify them, and people will have to look at the docs, or discover by trial and error that it is best to wait on fdc3Ready() beforehand.

@thorsent @kriswest @mattjamieson any thoughts on the above two options? I don't really mind either way.

Note that either of the two options above are just for ES6 import convenience in the npm package, and not suggested API spec changes - window.fdc3 and window.addEventListener('fdc3Ready') are still the underlying specification pieces.

[kriswest]

I too don't mind a whole lot either way. However, after thinking about it, option 1 makes the most sense to me as it's backwards compatible with <1.2 desktop agent implementations where the event did not exist (you just don't use `fdc3Ready()`). Granted developers should be using a release of the module that matches their FDC3 version, but then they may not be in control of that (e.g. a vendor whose app is used in different desktops). That will give them the scope to use the new getInfo() function (also synchronous, bad Kris!) or the fact it does not exist to determine the FDC3 version and use `fdc3Ready()` or nasty polling code. If you bake it in that module will only ever work with fdc3 1.2 and above implementations. Two other (off-topic) thoughts... - getInfo() has a slightly different usecase to other functions, but could/should it switch to async before people start using it? - the NPM module could make getInfo() more useful by determining fdc3 1.0 and 1.1 versions by a) checking for the existence of `getInfo()` and `getOrCreateChannel()` (if neither exist its 1.0, if only getInfo() doesn't exist its 1.1)

…

On Mon, 12 Apr 2021 at 15:58, Riko Eksteen ***@***.***> wrote: Thanks @thorsent <https://github.com/thorsent>. I do agree that making the three synchronous calls is counter-productive, I think in a future release, everything should be promisified. Doing this work has made clear to me how problematic those three functions that are synchronous are. I basically see two options: 1. Just keep the current code with rejects or throws if fdc3 isn't available, and recommend users to manually call fdc3Ready() function, once only (which has the added benefit of not running code which isn't necessary). The function still makes it slightly easier to handle the window event listener pattern, by wrapping it as a promise with a timeout. 2. Automatically call fdc3Ready() in all imported functions for safety of users who just do import { raiseIntent } from ***@***.***/fdc3' without understanding the nuances of the global object or ready event. This is with the understanding that it won't be possible for synchronous operations like broadcast *until* we promisify them, and people will have to look at the docs, or discover by trial and error that it is best to wait on fdc3Ready() beforehand. @thorsent <https://github.com/thorsent> @kriswest <https://github.com/kriswest> @mattjamieson <https://github.com/mattjamieson> any thoughts on the above two options? I don't really mind either way. Note that either of the two options above are just for ES6 import convenience in the npm package, and not suggested API spec changes - window.fdc3 and window.addEventListener('fdc3Ready') are still the underlying specification pieces. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#360 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM7PBDGDM3VS3HELXZXWNLTIMDAZANCNFSM42ZL5X7Q> .

-- Kris West Director of Solutions Engineering [image: Cosaic] +1 (800) 821-8147 ***@***.*** Cosaic.io: <https://cosaic.io/> ChartIQ <http://cosaic.io/ChartIQ> + Finsemble <http://cosaic.io/Finsemble>

[rikoe]

@kriswest agreed on both counts. Given that we have voted on the FDC3 1.2 release specification already, I am not sure we can change the signature now.

We should either reopen the voting, or wait until 2.0 and change, along with the other sync operators (I prefer the latter).

This does remind me that getInfo isn't currently exposed in the npm package though, will add it.

I will update PR to remove the fdc3Ready call inside exposed operators.

[kriswest]

LGTM
One minor suggestion on a comment.
Tests all pass, however, there is a complaint about an unhandled promise rejection that could be cleaned up/caught (not sure which test):

Running with --trace-warnings did not make this any clearer (npx cross-env NODE_OPTIONS="--trace-warnings" tsdx test --verbose)
Thank you for increasing the printWidth