-
Notifications
You must be signed in to change notification settings - Fork 17
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
RDP's in TS OSDK #1035
base: main
Are you sure you want to change the base?
RDP's in TS OSDK #1035
Changes from all commits
246f893
50ca838
4e17a4e
01f5935
ebe19e4
4ea27dc
d2d0a0d
efbed3d
40ee625
3e535da
20835ea
76a3ee1
73c23fe
d605787
214dad7
73fbffa
9070c29
ac7f03f
2777334
a8546ca
0cd49f5
0a89f9c
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 |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
"@osdk/shared.test": minor | ||
"@osdk/client": minor | ||
"@osdk/api": minor | ||
--- | ||
|
||
Adds support for runtime derived properties |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,10 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
import type { | ||
NumericWithPropAggregateOption, | ||
StringWithPropAggregateOption, | ||
} from "../derivedProperties/WithPropertyObjectSet.js"; | ||
import type { | ||
GetWirePropertyValueFromClient, | ||
} from "../mapping/PropertyValueMapping.js"; | ||
|
@@ -32,19 +36,23 @@ export type NumericAggregateOption = | |
| "approximateDistinct" | ||
| "exactDistinct"; | ||
|
||
type AGG_FOR_TYPE<T> = number extends T ? NumericAggregateOption | ||
: string extends T ? StringAggregateOption | ||
type AGG_FOR_TYPE<T, U extends boolean> = number extends T | ||
? U extends true ? NumericAggregateOption : NumericWithPropAggregateOption | ||
: string extends T | ||
? U extends true ? StringAggregateOption : StringWithPropAggregateOption | ||
: never; | ||
|
||
export type ValidAggregationKeys< | ||
Q extends ObjectOrInterfaceDefinition, | ||
R extends "aggregate" | "withPropertiesAggregate" = "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. @ssanjay1 Is this what you had in mind? 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. Yea that works 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. Though maybe keeping parity with the other names it should be |
||
> = keyof ( | ||
& { | ||
[ | ||
KK in AggregatableKeys<Q> as `${KK & string}:${AGG_FOR_TYPE< | ||
GetWirePropertyValueFromClient< | ||
CompileTimeMetadata<Q>["properties"][KK]["type"] | ||
> | ||
>, | ||
R extends "aggregate" ? true : false | ||
>}` | ||
]?: any; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* | ||
* Copyright 2024 Palantir Technologies, Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import type { | ||
ObjectOrInterfaceDefinition, | ||
} from "../ontology/ObjectOrInterface.js"; | ||
import type { ObjectMetadata } from "../ontology/ObjectTypeDefinition.js"; | ||
import type { BaseWithPropertyObjectSet } from "./WithPropertyObjectSet.js"; | ||
|
||
export type WithPropertyDefinition< | ||
T extends ObjectMetadata.Property, | ||
> = { | ||
definitionId: string; | ||
type: T; | ||
}; | ||
|
||
export type WithPropertiesClause< | ||
Q extends ObjectOrInterfaceDefinition, | ||
> = { | ||
[key: string]: ( | ||
baseObjectSet: BaseWithPropertyObjectSet<Q>, | ||
) => WithPropertyDefinition<ObjectMetadata.Property>; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
/* | ||
* Copyright 2024 Palantir Technologies, Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import type { ValidAggregationKeys } from "../aggregate/AggregatableKeys.js"; | ||
import type { WhereClause } from "../aggregate/WhereClause.js"; | ||
import type { | ||
ObjectOrInterfaceDefinition, | ||
PropertyKeys, | ||
} from "../ontology/ObjectOrInterface.js"; | ||
import type { | ||
CompileTimeMetadata, | ||
PropertyDef, | ||
} from "../ontology/ObjectTypeDefinition.js"; | ||
import type { LinkedType, LinkNames } from "../util/LinkUtils.js"; | ||
import type { WithPropertyDefinition } from "./WithPropertiesClause.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. I use inheritance here to make sure we enforce what methods are available and to reduce duplication |
||
export interface WithPropertyObjectSet<Q extends ObjectOrInterfaceDefinition> | ||
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. Lets be consistent with naming, see some |
||
extends | ||
BaseWithPropertyObjectSet<Q>, | ||
AggregatableWithPropertyObjectSet<Q>, | ||
SingleLinkWithPropertyObjectSet<Q> | ||
{} | ||
|
||
export interface BaseWithPropertyObjectSet< | ||
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. This object set only has filter and pivot as aggregations and selectProperty is not available on the base object set. |
||
Q extends ObjectOrInterfaceDefinition, | ||
> extends FilterableWithPropertyObjectSet<Q> { | ||
readonly pivotTo: <L extends LinkNames<Q>>( | ||
type: L, | ||
) => NonNullable<CompileTimeMetadata<Q>["links"][L]["multiplicity"]> extends | ||
false ? SingleLinkWithPropertyObjectSet<LinkedType<Q, L>> | ||
: ManyLinkWithPropertyObjectSet<LinkedType<Q, L>>; | ||
} | ||
|
||
interface FilterableWithPropertyObjectSet< | ||
Q extends ObjectOrInterfaceDefinition, | ||
> { | ||
readonly where: ( | ||
clause: WhereClause<Q>, | ||
) => this; | ||
} | ||
export type CollectWithPropAggregations = "collectSet" | "collectList"; | ||
|
||
export type BaseWithPropAggregations = | ||
| "approximateDistinct" | ||
| "exactDistinct" | ||
| "approximatePercentile"; | ||
|
||
export type StringWithPropAggregateOption = | ||
| BaseWithPropAggregations | ||
| CollectWithPropAggregations; | ||
|
||
export type NumericWithPropAggregateOption = | ||
| "min" | ||
| "max" | ||
| "sum" | ||
| "avg" | ||
| BaseWithPropAggregations | ||
| CollectWithPropAggregations; | ||
|
||
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. If I am thinking about whats ideal we need to add some type tests. I'm okay if its in a follow up |
||
interface AggregatableWithPropertyObjectSet< | ||
Q extends ObjectOrInterfaceDefinition, | ||
> { | ||
readonly aggregate: < | ||
V extends ValidAggregationKeys< | ||
Q, | ||
"withPropertiesAggregate" | ||
>, | ||
>( | ||
aggregationSpecifier: V, | ||
opts?: V extends `${any}:${infer P}` | ||
? P extends CollectWithPropAggregations ? { limit: number } | ||
: P extends "approximatePercentile" ? { percentile: number } | ||
: never | ||
: never, | ||
) => WithPropertyDefinition< | ||
V extends `${infer N}:${infer P}` | ||
? P extends CollectWithPropAggregations ? PropertyDef< | ||
CompileTimeMetadata<Q>["properties"][N]["type"], | ||
"nullable", | ||
"array" | ||
> | ||
: P extends "approximateDistinct" | "exactDistinct" | "$count" | ||
? PropertyDef<"integer"> | ||
: PropertyDef<"double"> | ||
: V extends "$count" ? PropertyDef<"integer"> | ||
: never | ||
>; | ||
} | ||
|
||
interface SingleLinkWithPropertyObjectSet< | ||
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.
|
||
Q extends ObjectOrInterfaceDefinition, | ||
> extends | ||
AggregatableWithPropertyObjectSet<Q>, | ||
FilterableWithPropertyObjectSet<Q>, | ||
BaseWithPropertyObjectSet<Q> | ||
{ | ||
readonly selectProperty: <R extends PropertyKeys<Q>>( | ||
propertyName: R, | ||
) => WithPropertyDefinition<CompileTimeMetadata<Q>["properties"][R]>; | ||
} | ||
|
||
interface ManyLinkWithPropertyObjectSet< | ||
Q extends ObjectOrInterfaceDefinition, | ||
> extends | ||
AggregatableWithPropertyObjectSet<Q>, | ||
FilterableWithPropertyObjectSet<Q> | ||
{ | ||
readonly pivotTo: <L extends LinkNames<Q>>( | ||
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. So just clarifying, the reason we need to duplicate 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. Yeah that's correct, which makes sense because for this to work you have to basically be navigating to exactly one object 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. We should probably document that here too. |
||
type: L, | ||
) => ManyLinkWithPropertyObjectSet<LinkedType<Q, L>>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,14 @@ export type { | |
OsdkObjectLinksObject, | ||
SingleLinkAccessor, | ||
} from "./definitions/LinkDefinitions.js"; | ||
export type { | ||
WithPropertiesClause, | ||
WithPropertyDefinition, | ||
} from "./derivedProperties/WithPropertiesClause.js"; | ||
export type { | ||
WithPropertyObjectSet, | ||
} from "./derivedProperties/WithPropertyObjectSet.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. Extra new line causes grouping when organizing imports instead of perfect sorting. |
||
export { DurationMapping } from "./groupby/GroupByClause.js"; | ||
export type { | ||
AllGroupByValues, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ import type { AggregateOpts } from "../aggregate/AggregateOpts.js"; | |
import type { AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy } from "../aggregate/AggregateOptsThatErrors.js"; | ||
import type { AggregationsResults } from "../aggregate/AggregationsResults.js"; | ||
import type { WhereClause } from "../aggregate/WhereClause.js"; | ||
import type { | ||
WithPropertiesClause, | ||
WithPropertyDefinition, | ||
} from "../derivedProperties/WithPropertiesClause.js"; | ||
import type { GetWirePropertyValueFromClient } from "../mapping/PropertyValueMapping.js"; | ||
import type { | ||
AsyncIterArgs, | ||
Augments, | ||
|
@@ -35,6 +40,7 @@ import type { | |
import type { | ||
CompileTimeMetadata, | ||
ObjectTypeDefinition, | ||
PropertyDef, | ||
} from "../ontology/ObjectTypeDefinition.js"; | ||
import type { PrimaryKeyType } from "../OsdkBase.js"; | ||
import type { ExtractOptions, Osdk } from "../OsdkObjectFrom.js"; | ||
|
@@ -254,4 +260,39 @@ export interface ObjectSet< | |
listener: ObjectSetListener<Q, P>, | ||
opts?: ObjectSetListenerOptions<Q, P>, | ||
) => { unsubscribe: () => void }; | ||
|
||
readonly withProperties: < | ||
D extends WithPropertiesClause<Q>, | ||
>( | ||
clause: D, | ||
) => ObjectSet< | ||
WithPropertiesObjectDefinition< | ||
Q, | ||
D | ||
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 okay with it for now but this feels like a place where having the simpler types that are not based on functions produces more readable types? We have to do the extraction work anyway to create things like |
||
> | ||
>; | ||
} | ||
|
||
type WithPropertiesObjectDefinition< | ||
K extends ObjectOrInterfaceDefinition, | ||
D extends WithPropertiesClause<K>, | ||
> = { | ||
__DefinitionMetadata: { | ||
properties: { | ||
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. This is all we need to propagate the types through all further object set operations. The FLUP here is to make the type of the whole object set extractable and definable so that a user can pass around object sets that are correctly typed with the additional properties and do not have to be inferred. |
||
[T in Extract<keyof D, string>]: D[T] extends | ||
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'd have to check but my gut is |
||
(baseObjectSet: any) => WithPropertyDefinition<infer P> ? P | ||
: never; | ||
}; | ||
props: { | ||
[T in Extract<keyof D, string>]: D[T] extends | ||
(baseObjectSet: any) => WithPropertyDefinition<infer P> | ||
? P extends PropertyDef<infer A, any, any> ? | ||
| GetWirePropertyValueFromClient< | ||
A | ||
> | ||
| undefined | ||
: never | ||
: never; | ||
}; | ||
}; | ||
} & K; |
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: rename
U
so its clear what that boolean generic is for