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

Question: adding BrowserTypes to root export #1338

Closed
1 of 6 tasks
Roaders opened this issue Sep 4, 2024 · 14 comments
Closed
1 of 6 tasks

Question: adding BrowserTypes to root export #1338

Roaders opened this issue Sep 4, 2024 · 14 comments
Labels
FDC3 for Web Browsers question Further information is requested

Comments

@Roaders
Copy link
Contributor

Roaders commented Sep 4, 2024

Question Area

  • App Directory
  • API
  • Context Data
  • Intents
  • Use Cases
  • Other

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
Copy link
Member

Adding to #1297

@kriswest
Copy link
Contributor

kriswest commented Sep 4, 2024

I've done the export in 065c0d7 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
Copy link
Member

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
Copy link
Contributor

kriswest commented Sep 5, 2024

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
Copy link
Contributor

kriswest commented Sep 5, 2024

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
Copy link
Contributor

kriswest commented Sep 5, 2024

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: f4f6009

@Roaders
Copy link
Contributor Author

Roaders commented Sep 5, 2024

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
Copy link
Member

robmoffat commented Sep 5, 2024

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
Copy link
Contributor

kriswest commented Sep 5, 2024

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

@kriswest
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member

I haven't republished yet

@kriswest
Copy link
Contributor

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

@kriswest
Copy link
Contributor

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

@kriswest kriswest added the question Further information is requested label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FDC3 for Web Browsers question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants