finos / FDC3

An open standard for the financial desktop.
https://fdc3.finos.org
Other
201 stars 132 forks source link

Question: adding BrowserTypes to root export #1338

Closed Roaders closed 2 months ago

Roaders commented 2 months ago

Question Area

Question

We've just moved over to the Beta release published under @kite9 and I noticed that I am still having to import messages from a deep path as BrowserTypes is not included in the root export:

import { RaiseIntentForContextRequest } from '@kite9/fdc3/dist/api/BrowserTypes';

What is the plan for including these in the root export? There are several naming collisions - do we have a plan for dealing with them?

robmoffat commented 2 months ago

Adding to #1297

kriswest commented 2 months ago

I've done the export in 065c0d78ba9ff011eb9c4616a8695890b446bbe8 as BrowserTypes (so the namespace should prevent conflicts) and made an adjustment to fix an existing name conflict with ContextTypes (which was not namespaced like this - although BridgingTypes was (many conflicts there), so there is precedent).

If this works for you @Roaders, then @robmoffat can just merge this in and tick it off his list for now.

robmoffat commented 2 months ago

Tagging @Lecss

This is great, thanks for tackling that @kriswest. One quick question - normally I wouldn't check in browserTypes as it is generated from other first-hand code (i.e. your schemas). What's the argument here for checking it in too? Or could we simply include the generated version in our npm package?

Interested to hear what @Roaders and @Lecss think on this too.

kriswest commented 2 months ago

We've always checked in the generated source file for ContextTypes.ts and BridgingTypes.ts, and do so on a pre-commit hook, as this allows us to review and track what is actually changing in the generated code. Were a quicktype update and tweak to the quicktype commands to alter the generated code we'd be able to see that, alongside changes caused by schema updates. Due to schema composition, changes in one schema can affect multiple of the generated source files and multiple elements within each file and due to that complexity, it can be hard for a contributor to know what will be affected by a change they implement in the schema files. Finally, its easy to inadvertently break the schema files through a syntax error or syntactically correct change that results in an unvalidatable schema. This is only revealed by running the code generation scripts (although we could do with further emphasizing the errors when they occur).

Hence, I see only benefits and no downsides to checking in the generated code.

kriswest commented 2 months ago

A getAgent() export would be the next one to add @robmoffat. I know the implementation team this end are keen to start working with it/testing against it ASAP.

kriswest commented 2 months ago

That reminds me, there is a /src/GetAgent.ts file with the type for the function and args:

export type getAgent = ( 
  params?: GetAgentParams,  
) => Promise<DesktopAgent>; 

But it should probably not be exported as getAgent - rather the implementation should be exported as getAgent in index.ts. I've gone ahead and renamed the type GetAgentType to avoid a conflict or confusion: f4f6009a5bfee84502dfcf7aa1a5343c0636f675

Roaders commented 2 months ago

Tagging @Lecss

This is great, thanks for tackling that @kriswest. One quick question - normally I wouldn't check in browserTypes as it is generated from other first-hand code (i.e. your schemas). What's the argument here for checking it in too? Or could we simply include the generated version in our npm package?

Interested to hear what @Roaders and @Lecss think on this too.

In this case I think checking it in would be useful so that it's visible in GitHub and people can look at it there. Generally I do not like checking in generated stuff but I think this would be an exception

robmoffat commented 2 months ago

A getAgent() export would be the next one to add @robmoffat. I know the implementation team this end are keen to start working with it/testing against it ASAP.

You can use this. Add the following deps to package.json:

   "@kite9/client": "2.2.0-beta.6",
    "@kite9/fdc3": "2.2.0-beta.6",

And then just import:

import { getAgent } from '@kite9/client'

We'll move back to the @finos namespace once the vote goes through, as discussed in a meeting earlier this week

kriswest commented 2 months ago

@flashd2n (Kalin), @ksgeorgieva (Kalina) see the above comment from @robmoffat on access to the draft getAgent() implementation

kriswest commented 2 months ago

@robmoffat did you merge the BrowserTypes export in index.ts and republish? I.e. can we close this as completed? At https://unpkg.com/browse/@kite9/fdc3@2.2.0-beta.6/dist/fdc3.cjs.development.js I see ContextTypes and BridgingTypes, but not yet BrowserTypes so I would assume not.

robmoffat commented 2 months ago

yes, I've done this in fdc3-for-web-impl. I'd really like to take you through it as I'm not 100% confident with all these name collisions and picking the right ones

robmoffat commented 2 months ago

I haven't republished yet

kriswest commented 2 months ago

I'm around shortly, and can connect with you to take a look @robmoffat

kriswest commented 2 months ago

Closed as completed - see @robmoffat's message above about getting at a sample, which will move into main when its been reviewwed and we've confirmed that FDC3 for the Web is going into the next FDC3 version at SWG meeting on 26th September