-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add structs read support #1047
Add structs read support #1047
Changes from 4 commits
8951fb6
7d5605c
71dcfb2
a8be10e
b6ae16d
5628328
1de9dcd
d7b6ec9
20adee1
fa6013e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,13 @@ export interface PropertyValueWireToClient { | |
geotimeSeriesReference: GeotimeSeriesProperty<GeoJSON.Point>; | ||
} | ||
|
||
export type getClientPropertyValueFromWire< | ||
T extends | ||
| keyof PropertyValueWireToClient | ||
| Record<string, keyof PropertyValueWireToClient>, | ||
> = T extends keyof PropertyValueWireToClient ? PropertyValueWireToClient[T] | ||
: T; | ||
|
||
/** | ||
* Map from the PropertyDefinition type to the typescript type that we accept | ||
*/ | ||
|
@@ -71,3 +78,9 @@ export interface PropertyValueClientToWire { | |
sensorTimeseries: TimeSeriesProperty<string | number>; | ||
geotimeSeriesReference: GeotimeSeriesProperty<GeoJSON.Point>; | ||
} | ||
export type getWirePropertyValueFromClient< | ||
T extends | ||
| keyof PropertyValueClientToWire | ||
| Record<string, keyof PropertyValueClientToWire>, | ||
> = T extends keyof PropertyValueClientToWire ? PropertyValueClientToWire[T] | ||
: T; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,10 @@ | |
*/ | ||
|
||
export type WirePropertyTypes = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
| Record<string, SimpleWirePropertyTypes>; | ||
|
||
export type SimpleWirePropertyTypes = | ||
| "string" | ||
| "datetime" | ||
| "double" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,10 @@ | |
import type { | ||
AggregateOpts, | ||
AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy, | ||
CompileTimeMetadata, | ||
GroupByClause, | ||
ObjectOrInterfaceDefinition, | ||
PropertyKeys, | ||
ValidAggregationKeys, | ||
} from "@osdk/api"; | ||
import type { Employee } from "@osdk/client.test.ontology"; | ||
|
@@ -37,6 +40,9 @@ import { | |
type Mock, | ||
vi, | ||
} from "vitest"; | ||
import type { getWirePropertyValueFromClient } from "../../../api/build/esm/mapping/PropertyValueMapping.js"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bad import There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good catch |
||
import type { Client } from "../Client.js"; | ||
import { createClient } from "../createClient.js"; | ||
import { createMinimalClient } from "../createMinimalClient.js"; | ||
import type { MinimalClient } from "../MinimalClientContext.js"; | ||
import { aggregate } from "./aggregate.js"; | ||
|
@@ -50,6 +56,7 @@ const metadata = { | |
|
||
let mockFetch: Mock; | ||
let clientCtx: MinimalClient; | ||
let client: Client; | ||
|
||
beforeAll(() => { | ||
mockFetch = vi.fn(); | ||
|
@@ -63,10 +70,18 @@ beforeAll(() => { | |
clientCtx = createMinimalClient( | ||
metadata, | ||
"https://host.com", | ||
async () => "", | ||
async () => "myAccessToken", | ||
{}, | ||
mockFetch, | ||
); | ||
|
||
client = createClient( | ||
"https://host.com", | ||
metadata.ontologyRid, | ||
async () => "", | ||
undefined, | ||
mockFetch, | ||
); | ||
}); | ||
|
||
const aggregationResponse: AggregateObjectsResponseV2 = { | ||
|
@@ -171,13 +186,7 @@ describe("aggregate", () => { | |
> | ||
>(false); // subSelect should hide unused keys | ||
|
||
const grouped = await aggregate( | ||
clientCtx, | ||
objectTypeWithAllPropertyTypes, | ||
{ | ||
type: "base", | ||
objectType: "ToDo", | ||
}, | ||
const grouped = await client(objectTypeWithAllPropertyTypes).aggregate( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, that seems off. what kind of issues? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: { | ||
"id:approximateDistinct": "unordered", | ||
|
@@ -509,13 +518,7 @@ describe("aggregate", () => { | |
}); | ||
|
||
it("prohibits ordered select with multiple groupBy", async () => { | ||
aggregate( | ||
clientCtx, | ||
Todo, | ||
{ | ||
type: "base", | ||
objectType: "ToDo", | ||
}, | ||
client(Todo).aggregate( | ||
{ | ||
$select: { | ||
// @ts-expect-error | ||
|
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 find this type being lower case to be a bit confusing
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.
Ah no I agree, bad convention on my part