Add structs read support #1047
Add structs read support #1047
Conversation
| type: "base", | ||
| objectType: "ToDo", | ||
| }, | ||
| const grouped = await client(objectTypeWithAllPropertyTypes).aggregate( |
There was a problem hiding this comment.
For some reason type tests were having issues when using the aggregate function directly, but were all good when using the user exposed aggregate function. This should be fine since the user facing stuff works, but just flagging
There was a problem hiding this comment.
hmm, that seems off. what kind of issues?
There was a problem hiding this comment.
For some reason, the aggregate function was not capturing clause types correctly and everything was still in generic form (e.g. like it wasn't getting the concrete information passed in $select: {myProp:asc}
| @@ -46,6 +46,8 @@ export function wirePropertyV2ToSdkPrimaryKeyTypeDefinition( | |||
| case "float": | |||
| case "geotimeSeriesReference": | |||
| case "mediaReference": | |||
| case "struct": | |||
There was a problem hiding this comment.
I know we want to ignore these property types instead of throwing errors for unsupported, but saving as a separate PR since structs is higher prio.
| structMap[structField.apiName] = | ||
| objectPropertyTypeToSdkPropertyDefinition( | ||
| structField.dataType, | ||
| ) as SimpleWirePropertyTypes; |
There was a problem hiding this comment.
This is fine because right now, structs can only be field -> simple property type, cannot have nested structs yet in OSS
| type: "struct", | ||
| structFieldTypes: Object.entries(t.type).map(([apiName, dataType]) => ({ | ||
| apiName, | ||
| dataType: { type: dataType } as ObjectPropertyType, |
There was a problem hiding this comment.
This cast should also be fine because the maker package only uses this for primary key types, which are limited to a select few primitive types
| @@ -37,6 +40,9 @@ import { | |||
| type Mock, | |||
| vi, | |||
| } from "vitest"; | |||
| import type { getWirePropertyValueFromClient } from "../../../api/build/esm/mapping/PropertyValueMapping.js"; | |||
| type: "base", | ||
| objectType: "ToDo", | ||
| }, | ||
| const grouped = await client(objectTypeWithAllPropertyTypes).aggregate( |
There was a problem hiding this comment.
hmm, that seems off. what kind of issues?
| @@ -71,3 +78,9 @@ export interface PropertyValueClientToWire { | |||
| sensorTimeseries: TimeSeriesProperty<string | number>; | |||
| geotimeSeriesReference: GeotimeSeriesProperty<GeoJSON.Point>; | |||
| } | |||
| export type getWirePropertyValueFromClient< | |||
There was a problem hiding this comment.
I find this type being lower case to be a bit confusing
There was a problem hiding this comment.
Ah no I agree, bad convention on my part
| @@ -15,6 +15,10 @@ | |||
| */ | |||
|
|
|||
| export type WirePropertyTypes = | |||
There was a problem hiding this comment.
Should this be WireAndStructPropertyTypes? That way we don't break WirePropertyTypes?
There was a problem hiding this comment.
Talked a bit on slack, just to clarify here as well, the reason you think this is a break is because now instead of just a list of strings, its SimpleWirePropertyTypes and the record type? Because technically this boils down to the same thing and is just an additive change, but if someone is using this type, could be confusing since now it referencs something called SimpleWirePropertyTypes?
| | keyof PropertyValueClientToWire | ||
| | Record<string, keyof PropertyValueClientToWire>, | ||
| > = T extends keyof PropertyValueClientToWire ? PropertyValueClientToWire[T] | ||
| : T; |
There was a problem hiding this comment.
I'm being a bit silly but I'm not understanding why this type works. Why doesn't it need to be nested to be kind of like T extends keyof PropertyValueClientToWire ? PropertyValueClientToWire[T] : {[K in T]: PropertyValueClientToWire[K]};
There was a problem hiding this comment.
mmm actually, this is a good point. We should still remap the inner types of the struct. We mainly use this type for agg results which isn't supported for structs yet, but I added some tests to make sure this works as intended
No description provided.