-
Notifications
You must be signed in to change notification settings - Fork 20
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
Adds an example for subscriptions #990
base: main
Are you sure you want to change the base?
Changes from 1 commit
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,39 @@ | ||
/* | ||
* 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, Osdk } from "@osdk/api"; | ||
|
||
export function consolidateOsdkObject< | ||
T extends Osdk.Instance<V, any, any>, | ||
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. Why are you capturing 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 capture them separately since I want to specify the return type to be one or the other. I want the ability for someon to pass in 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. The bigger question I have here is which one should we return. I think we should always scope the object down to the oldObject. So if oldObject has a more narrow definition, we want the oldDefinition to be returned and also delete extraneous keys off of the new 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. The edge case that's left out here is if someone wants an object scoped down to Object B. They can't just reverse what they put in since we explicitly use the properties on the latter 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. I think this exclusion is ok. In my mind, this can be used for loadObject and subscriptions, where they already have a defined store. For either, they will likely want an object in the shape of that store instead of what they receive necessarily on the load or subscribe. I can't really imagine why they would want the edge case described above 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 so, maybe we rename as such then. I was confused because earlier it seemed like you were just expanding to whatever the new return type was 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 just need this name to be clear that you're updating existing properties, without expanding the property scope of the old object (even if the new object has more props) |
||
U extends Osdk.Instance<V, any, any>, | ||
V extends ObjectOrInterfaceDefinition, | ||
>( | ||
oldObject: T | undefined, | ||
upToDateObject: U, | ||
): U { | ||
if (!oldObject) { | ||
return upToDateObject; | ||
} | ||
const combinedObject = oldObject as any; | ||
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. Do we need this 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. 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 when I update the return type, I'm going to remove this any cast and just iterate on object B based on the keys of object A and update the old object. |
||
|
||
for (const key in upToDateObject) { | ||
if (upToDateObject.hasOwnProperty(key)) { | ||
combinedObject[key] = upToDateObject[key]; | ||
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. Hmmm, also I think we may need to update the internal values here right? Not totally sure so double check me, but if someone starts like casting things |
||
} | ||
} | ||
|
||
return combinedObject; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
* 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 { Osdk } from "@osdk/api"; | ||
import type { Employee, Todo } from "@osdk/client.test.ontology"; | ||
import { describe, expect, expectTypeOf, it } from "vitest"; | ||
import { consolidateOsdkObject } from "./consolidateOsdkObject.js"; | ||
|
||
describe(consolidateOsdkObject, () => { | ||
it("combines two objects", () => { | ||
const oldObject: Osdk.Instance<Todo, never, "text"> = { | ||
$apiName: "Todo", | ||
$objectType: "type", | ||
$primaryKey: 1, | ||
$title: "Employee", | ||
text: "text", | ||
} as any; | ||
|
||
const upToDateObject: Osdk.Instance<Todo> = { | ||
$apiName: "Todo", | ||
$objectType: "type", | ||
$primaryKey: 1, | ||
$title: "Employee", | ||
id: 1, | ||
text: "hi", | ||
} as any; | ||
|
||
const result = consolidateOsdkObject(oldObject, upToDateObject); | ||
|
||
expectTypeOf(result).toEqualTypeOf<Osdk.Instance<Todo>>(); | ||
|
||
expect(result).toMatchInlineSnapshot(` | ||
{ | ||
"$apiName": "Todo", | ||
"$objectType": "type", | ||
"$primaryKey": 1, | ||
"$title": "Employee", | ||
"id": 1, | ||
"text": "hi", | ||
} | ||
`); | ||
}); | ||
}); |
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.
Not used anymore, will delete