-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat(common): updates to analyticsWaveApi typings #566
feat(common): updates to analyticsWaveApi typings #566
Conversation
/** | ||
* Validates an Analytics template for org readiness. | ||
* | ||
* https://developer.salesforce.com/docs/component-library/documentation/en/lwc/reference_analytics_validate_wave_template |
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.
Note: some of these docs are not yet pushed out but will be, and/or will start working once the docs switch to 58.0 as the default.
/** The task specific message. */ | ||
message: string | null; | ||
/** The status for the readiness task */ | ||
readinessStatus: string | null; |
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.
Since this is like an enum, could it use string literals in the type, like readinessStatus: "failed" | "success" | null
(just making up those values, not sure if they are the right ones)?
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.
These types come from the generated adapter code, and are basically copied here. And, we need to match what the adapter code says, since any usages of this in js will call through the adapter. In the adapter code, it (probably) intentionally doesn't put union types for underlying enums, but just does string
, so we do that here, too. We sometimes put a note in the jsdoc about the possible values, but generally just rely on the docs, since those get updated per release.
*/ | ||
export interface TemplateReadinessItemRepresentation { | ||
/** The icon/image associated with the validation task. */ | ||
image: TemplateAssetReferenceRepresentation | null; |
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.
Maybe sorta a nit, but the TemplateAssetReferenceRepresentation
type and some others I see in this file have their properties marked optional (blah?: type
) but this one has the properties as possibly null
?
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.
This is intentional, and again from the generated adapter typings. The adapter runtime does differentiate between optional vs. required-but-can-be-null, and it does match up with what the response from the server code will have. In this case, image
will always be in the response json but can be null, which is why it's this way.
* id (string): id | ||
*/ | ||
export interface TemplateValidateRepresentation { | ||
/** The ID or fully qualified API name of this template. */ |
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.
Nit: is this comment right? It can be id or name?
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.
Yep. A number of our rest apis (and therefore adapters) support that in the urls and then the resulting output json.
What does this PR do?
ids
togetDatasets
validateWaveTemplate
These are for changes made the adapters in 58.0.
What issues does this PR fix or reference?
@W-13089840@
cc: @mblumreich