ihe stack 4/4 - DR Response #1405
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jonahkaye
requested review from
Goncharo,
leite08,
Orta21 and
RamilGaripov
as code owners
| if (isBaseErrorResponse(response)) { | ||
| status = "failure"; | ||
| } else if (isDocumentQueryResponse(response) || isDocumentRetrievalResponse(response)) { | ||
| if (isDocumentQueryResponse(response) || isDocumentRetrievalResponse(response)) { |
There was a problem hiding this comment.
I changed the order here. Why? Because of type narrowing! After some research:
In TypeScript, the process of refining the type of a variable within a certain scope based on conditions or type guards is known as "type narrowing". This feature helps prevent errors by ensuring that we only use properties and methods that are guaranteed to exist on the narrowed type.
In the original code, we first checked if response is a BaseErrorResponse using the isBaseErrorResponse type guard. If this condition was false, TypeScript narrowed the type of response to exclude BaseErrorResponse in the subsequent else if block. However, because BaseErrorResponse is not explicitly part of the IHEResult union, TypeScript inferred that response is of type never in the else if block. The never type in TypeScript represents a value that never occurs, which resulted in me getting an error when trying to access response.documentReference.
To resolve this, I rearranged the conditions to first check if response is a DocumentQueryResponse or DocumentRetrievalResponse. These types are part of the IHEResult union, so TypeScript can correctly infer the type of response in each block. This change ensures that we are correctly narrowing the type of response and prevents the TypeScript error.
There was a problem hiding this comment.
Yeah, always from the more specific to the more generic when checking for types.
| @@ -30,7 +30,7 @@ export const SamlAttributesSchema = z.object({ | |||
|
|
|||
| export const baseRequestSchema = z.object({ | |||
| id: z.string(), | |||
| cxId: z.string(), | |||
| cxId: z.string().optional(), | |||
There was a problem hiding this comment.
making optional because requests from the outside world will not have a cxID
There was a problem hiding this comment.
Let's then use a diff base schema for those? It will be noisy/cumbersome to have optional cxId for our internal code when we know it always must be present.
|
🎉 This PR is included in version 5.49.0-develop.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
| }, | ||
| }); | ||
| // call convertToDomainObject(response) here | ||
| const patients: Patient[] = response.data.map((patient: PatientDTO) => |
There was a problem hiding this comment.
.data is any, so it might be undefined = potential runtime error
Since here, check other similar code in this file please, there are more of those
| const message = "Failed with the call to update the doc-ref of an uploaded file"; | ||
| console.log(`${message}: ${docRef}`); |
There was a problem hiding this comment.
Do we really want to fail silently here?
| @@ -30,7 +30,7 @@ export const SamlAttributesSchema = z.object({ | |||
|
|
|||
| export const baseRequestSchema = z.object({ | |||
| id: z.string(), | |||
| cxId: z.string(), | |||
| cxId: z.string().optional(), | |||
There was a problem hiding this comment.
Let's then use a diff base schema for those? It will be noisy/cumbersome to have optional cxId for our internal code when we know it always must be present.
|
🎉 This PR is included in version 5.49.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Ticket: #1379, #1358
Dependencies
Description
Testing
TODO
Release Plan