-
Notifications
You must be signed in to change notification settings - Fork 214
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
Fix GraphicRepresentation types according to latest Mesh Export API docs #7570
base: master
Are you sure you want to change the base?
Conversation
common/api/frontend-tiles.api.md
Outdated
@@ -88,18 +88,18 @@ export type GraphicRepresentation = { | |||
}); | |||
|
|||
// @beta | |||
export type GraphicRepresentationFormat = "IMDL" | "3DTILES" | string; | |||
export type GraphicRepresentationFormat = "IMODEL" | "3DTiles" | "CESIUM"; |
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.
@danieliborra @arvindsvenkat To clarify, is "3DFT" as an export type still returned by the service? If so I'll add it here
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.
Looks good to me.
I will defer to @danieliborra and @arvindsvenkat for final approval, though.
Do we need to notify anyone of this change?
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.
will this be needed in 4.10 or 4.11?
@@ -92,7 +92,7 @@ export async function* queryMeshExports(args: QueryMeshExportsArgs): AsyncIterab | |||
changeId: args.changesetId, | |||
type: "IMODEL", | |||
}, | |||
format: "IMDL", | |||
format: "IMODEL" as GraphicRepresentationFormat, |
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.
why is the cast here necessary?
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.
Without the cast the compiler was complaining about the type of format
being incorrect, inferred to be string instead of GraphicRepresentationFormat. Explicitly stating the type is wordier but is that preferred? E.g.
const testArgs: {
accessToken: string;
sessionId: string;
dataSource: {
iTwinId: string;
id: string;
changeId?: string;
type: string;
};
format: GraphicRepresentationFormat;
} = {
accessToken: "this-is-a-fake-access-token",
sessionId: "testSession",
dataSource: {
iTwinId: "iTwinId",
id: "srcId",
changeId: undefined,
type: "srcType",
},
format: "IMODEL",
};
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.
while wordier, i think its safer to extract the type out for an internal interface to use
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.
Fixed
|
||
/** Graphic representations are generated from Data Sources. | ||
* The status of a Graphic Representation indicates the progress of that generation process. | ||
* @beta | ||
*/ | ||
// ###TODO this needs to be expanded to include more statuses, and/or "failed" needs to be replaced with "invalid". | ||
export enum GraphicRepresentationStatus { |
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.
curious why in one place we're using a string literal type and another place an enum
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.
I don't completely remember, but my guess is because GraphicRepresentationFormat used to have enum values that were strings different from the constant names, like this:
export enum GraphicRepresentationStatus {
InProgress = "In progress",
Complete = "Complete",
NotStarted = "Not started",
Failed = "Failed"
}
Why those different strings were necessary to begin with, I'm not sure and it could have just been a poor decision. Since all that is needed are the strings "InProgress", "Complete", "NotStarted", and "Invalid", it could become a type like GraphicRepresentationFormat
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.
I realized one reason we did this is the enum is used in a conditional type definition of GraphicRepresentation here:
export type GraphicRepresentation = {
displayName: string;
representationId: string;
status: GraphicRepresentationStatus;
format: GraphicRepresentationFormat;
dataSource: DataSource;
/** The url of the graphic representation
* @note The url can only be guaranteed to be valid if the status is complete.
* Therefore, the url is optional if the status is not complete, and required if the status is complete.
*/
} & ({
status: Omit<GraphicRepresentationStatus, GraphicRepresentationStatus.Complete>;
url?: string;
} | {
status: GraphicRepresentationStatus.Complete;
url: string;
});
Where the url
prop is only guaranteed to exist if status = GraphicRepresentationStatus.Complete. This could also be done with a union type though with Exclude<GraphicRepresentationStatus, "Complete">
, etc. instead of Omit
.
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.
@aruniverse do you have a preference between enum and string literal type here?
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.
I personally would prefer the string literals, not a huge fan of enums. I think the Exclude utility would be perfect for the discriminated union type
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.
Also fixed this
No, we were planning for it to go in 5.0. |
}, | ||
}; | ||
let graphicRepresentation: GraphicRepresentation; | ||
if (foundSource.status === "Complete") { |
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.
Updated the GraphicRepresentationFormat
type to be a union of strings, which unfortunately requires this long if statement to enforce that url
has type string
only if status === "Complete"
, and type string | undefined
otherwise. I tried to do this with a ternary operator but that doesn't work so I think this is the best solution to avoid more type assertions.
Update
GraphicRepresentationFormat
type andGraphicRepresentationStatus
enum to reflect the latest Mesh Export Service docs. These are breaking changes to beta APIs intended to go into iTwin.js 5.0.