search

finos / FDC3

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

remove union type for chat message #1102

Closed Yannick-Malins closed 6 months ago

Yannick-Malins commented 7 months ago

force the message part of a chat message to be a types structure, remove the union type that was kept for initial compatibility

update documentation

linux-foundation-easycla[bot] commented 7 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

netlify[bot] commented 7 months ago

Deploy Preview for fdc3 canceled.

Name Link
Latest commit 0505a4d43f7a38772b3ff2130efcd2c5ee209cb6
Latest deploy log https://app.netlify.com/sites/fdc3/deploys/6579b227d8d44e0008d7a657
kriswest commented 7 months ago

@bingenito Heres a PR from @Yannick-Malins that will remove that union type, which was left in for backwards compatibility, but is likely not needed as this type was added in 2.1. If this looks good to you we'll confirm consent for it at the next SWG meeting and then merge it (and regenerate the typescript context types in the NPM module - currently a manual process due to a quicktype issue).

I'll also see what we can do about adding a recommendation to not use union types in future.

bingenito commented 7 months ago

(and regenerate the typescript context types in the NPM module - currently a manual process due to a quicktype issue).

Is it possible to see what the type will look like as part of the PR?

kriswest commented 7 months ago

(and regenerate the typescript context types in the NPM module - currently a manual process due to a quicktype issue).

Is it possible to see what the type will look like as part of the PR?

Not until I can migrate it to FINOS repo branch and run the generation (would be automated, but for the aforementioned quicktype issue). I can do that migration after @Yannick-Malins is authorized by easyCLA (which is hopefully just a case of clicking on the link in the above message from the easyCLA bot https://github.com/finos/FDC3/pull/1102#issuecomment-1805373038)

kriswest commented 7 months ago

This is the quicktype PR we're waiting on seeing merged so we can go back to automated generation: https://github.com/glideapps/quicktype/pull/2426 I've given them a nudge on it

kriswest commented 7 months ago

@bingenito I got access to the fork from @Yannick-Malins to regenerate the types, you can now see the change to ContextTypes.ts in the diff

bingenito commented 7 months ago

Thanks @kriswest and @Yannick-Malins, looks good. I could have inferred that is the result but nice to see what gets generated up front.

kriswest commented 7 months ago

Thanks @kriswest and @Yannick-Malins, looks good. I could have inferred that is the result but nice to see what gets generated up front.

Very true - feel free to bug the quicktype maintainers for me :-p

kriswest commented 7 months ago

Generation of types is back to automated on commit, although it doesn't make it into the PR unless you run it yourself first, which is now also possible.

@Yannick-Malins the SWG meeting approved doing this last week, so now we just need to:

kriswest commented 6 months ago

/easycla

kriswest commented 6 months ago

@Yannick-Malins @mistryvinay can one of you hit that authorize link in the EasyCLA comment and check if you've got one of the new CLAs in place?