-
Notifications
You must be signed in to change notification settings - Fork 131
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
734 return app identifier and add function to retrieve app metadata #751
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 canceled.
|
One question: Is AppMetadata just a subset of the AppD record in this version? In #734 we were discussing making them equivalent, I think. |
It remains a subset. TBH, I think it will always be a subset (as it may not
be appropriate to return the details and hostManifests elements), although
I would like to widen it some (to include the email addresses, moreInfo
link, publisher, lang and localizedVersions info).
K
…On Mon, 13 Jun 2022 at 10:04, Rob Moffat ***@***.***> wrote:
One question:
Is AppMetadata just a subset of the AppD record in this version? In #734
<#734> we were discussing making them
equivalent, I think.
—
Reply to this email directly, view it on GitHub
<#751 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM7PBHPAPFW2UP326KACS3VO32RFANCNFSM5YOB5G4A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Kris West
Principal Engineer
[image: Cosaic]
+1 (800) 821-8147
***@***.***
Cosaic.io: <https://cosaic.io/> ChartIQ <http://cosaic.io/ChartIQ> +
Finsemble <http://cosaic.io/Finsemble>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kriswest, LGTM. A few tiny tweaks.
Co-authored-by: Matt Jamieson <10372+mattjamieson@users.noreply.github.com>
well spotted on a few of those, cheers @mattjamieson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@openfin-johans @openfin-gavin @MichaelMCoates any chance you guys could take a look at this today? I'd like to get it off to the mailing list ASAP so we can then get on with sending out the pre-draft for the adoption vote next week. The aim of the issue/PR is to reduce the need for the container to reproduce the full AppMetadata (which is growing in size gradually) in situations where it often isn't needed (for example on every context message or IntentResolution) and to provide a way to retrieve it. This should be more efficient for the container and clearer for the developer in terms of workflow (if you need the display metadata for an app, go retrieve it). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a peach!
resolves #734
Adds a new function to retrieve
AppMetadata
for anAppIdentifier
and reduces several other return types to only specifyAppIdentifier
rather thanAppMetadata
(which is a superset ofAppIdentifier
's fields). This helps clarify what fields developers can expect in returns from functions (i.e. only the minimal set inAppIdentifier
) and that they should manually retrieve any additional metadata (i.e. anAppMetadata
) if they need it (for display purposes).Key areas to review (preview links):
getAppMetadata()
fn reference: https://deploy-preview-751--lambent-kulfi-cf51a7.netlify.app/docs/next/api/ref/desktopagent#getappmetadataAppIdentifier
fromAppMetadata
:ContextMetadata
IntentResolution
findInstances
open
Finally, please note that the
AppIntent
returned byfindIntent
still usesAppMetadata
(as the use case will pretty much always require it) as does theImplementationMetadata
returned bygetInfo
.