Timeseries #435

ssanjay1 · 2024-07-02T16:42:32Z

Add timeseries functionality for 2.0 syntax

ssanjay1 · 2024-07-18T13:33:25Z

examples-extra/basic/cli/src/runTimeseriesTest.ts

+import { DherlihyComplexObject } from "@osdk/examples.basic.sdk";
+import { client } from "./client.js";
+
+export async function runTimeseriesTest() {


Added these just to hit real stack

ssanjay1 · 2024-07-18T14:10:53Z

packages/client.api/src/OsdkObjectFrom.ts

@@ -139,7 +140,7 @@ export type Osdk<
    /** @deprecated use $primaryKey */
    __primaryKey: Q extends ObjectTypeDefinition<any>
      ? OsdkObjectPrimaryKeyType<Q>
-      : unknown;
+      : any;


Had to type this as any because unknown doesn't work when we have an interface type. It used to work when PropertyValueClientToWire basically also evaluated to unknown, so it'd be an unknown to unknown check, but now its unknown to string|boolean|number|attachment...

Primary key is always string | number in the fallback case, no? Exposing an any here is sure to create pain for someone.

ssanjay1 · 2024-07-18T14:14:52Z

packages/client/src/createTimeseriesProperty.ts

+    }
+    : {
+      type: "relative",
+      endTime: {


It made the most sense to me to set this as an endtime if the query was after, e.g. give me all the high temperatures 2 weeks after today, will give all the temps from today up to 2 weeks later

Makes sense. Can we add an invariant check in this function when not in production mode too? That would help anyone using pure JS (or throwing as any at their code) as well as serve as an extra validation to our own logic.

ssanjay1 · 2024-07-18T14:15:30Z

packages/client/src/object/convertWireToOsdkObjects.test.ts

@@ -167,7 +167,7 @@ describe("convertWireToOsdkObjects", () => {
      clientCtx,
      [objectFromWire],
      undefined,
-    )) as Osdk<Employee>[];
+    )) as unknown as Osdk<Employee>[];


Had to do this because Osdk<> type is pretty complex now so it has a hard time casting from Osdk<ObjectOrInterfaceDefinition> to Osdk<myConcreteType>

ssanjay1 · 2024-07-18T14:15:48Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkInterface.ts

@@ -50,7 +50,7 @@ export function createOsdkInterface<
  };

  if (process.env.TARGET !== "browser") {
-    Object.setPrototypeOf(interfaceHolder, OsdkCustomInspectPrototype);
+    Object.setPrototypeOf(interfaceHolder, null);


So this is what we discussed, are we still good with this for now?

Lets revert this change for now. It only makes sense to do this in non-browser. If it rears its head again, we can come up with a better long term solution.

ssanjay1 · 2024-07-18T14:16:07Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkObject.ts

@@ -38,7 +39,7 @@ import type { PropertyDescriptorRecord } from "./PropertyDescriptorRecord.js";
 const objectPrototypeCache = createClientCache(
  function(client, objectDef: FetchedObjectTypeDefinition<any, any>) {
    return Object.create(
-      process.env.target !== "browser" ? OsdkCustomInspectPrototype : null,
+      process.env.target !== "browser" ? null : null,


Can remove the ternary if we just decide to set the prototype to null

Lets restore this one too

ssanjay1 · 2024-07-18T14:16:27Z

packages/client/src/object/object.test.ts

@@ -58,7 +58,18 @@ describe("OsdkObject", () => {

      // we should get the employee we requested
      const employee = result.data[0];
-      expect(employee).toEqual(asV2Object(stubData.employee1));


Had to revise this because the employeeStatus property now gets functions

ssanjay1 · 2024-07-18T14:16:55Z

packages/client/src/objectSet/ObjectSet.test.ts

-        },
-      ]
-    `);
+    expect(employees).toMatchObject([


Test checks the same things, just revised so I could tell it to expect anything for the timeseries property

ssanjay1 · 2024-07-18T14:17:17Z

packages/client/src/util/streamutils.ts

+const OBJECT_OPEN_CHAR_CODE = 123; // '{'
+const OBJECT_CLOSE_CHAR_CODE = 125; // '}'
+
+export async function* parseStreamedResponse(


Pulled this parsing helper from the legacy implementation

Cool with this for now as we know it works but I'm curious if there is something a bit less custom that works out of the box for browsers? Def for a follow up.

ericanderson · 2024-07-18T14:52:10Z

packages/client.api/src/Definitions.ts

+import {
+  type ObjectTypePropertyDefinition,
+  WirePropertyTypes,
+} from "@osdk/api";


Nit: Unnecessary change?

ericanderson · 2024-07-18T14:53:26Z

packages/client.api/src/OsdkObjectFrom.ts

@@ -139,7 +140,7 @@ export type Osdk<
    /** @deprecated use $primaryKey */
    __primaryKey: Q extends ObjectTypeDefinition<any>
      ? OsdkObjectPrimaryKeyType<Q>
-      : unknown;
+      : any;


Primary key is always string | number in the fallback case, no? Exposing an any here is sure to create pain for someone.

ericanderson · 2024-07-18T14:54:20Z

packages/client.api/src/index.ts

@@ -14,6 +14,8 @@
 * limitations under the License.
 */

+import { TimeseriesDurationMapping } from "./timeseries/timeseries.js";


Why do we need this import?

Ah we very much do not, good catch

ericanderson · 2024-07-18T14:58:17Z

packages/client/src/createTimeseriesProperty.ts

+  primaryKey: any,
+  propertyName: string,
+): TimeSeriesProperty<T> {
+  return {


Nit: Can we convert the implementation of TimeSeriesProperty into a class here (either in this PR or in a followup). It will be a tiny improvement to memory/runtime.

Sure will FLUP

ericanderson · 2024-07-18T15:00:47Z

packages/client/src/createTimeseriesProperty.ts

+  };
+}
+
+async function getAllTimeSeriesPoints<T extends string | number>(


A few questions:

Why isn't this function implemented by calling iterateTimeSeriesPoints? It feels duplicative and hard to maintain as two parallel code paths.

Should we be exposing a getAll here?

Totally right, I'll do that

We had a getAll in legacy so I exposed for parity, I don't have enough data on usage here though to see if thats how people use it or not

ericanderson · 2024-07-18T15:03:23Z

packages/client/src/createTimeseriesProperty.ts

+    }
+    : {
+      type: "relative",
+      endTime: {


Makes sense. Can we add an invariant check in this function when not in production mode too? That would help anyone using pure JS (or throwing as any at their code) as well as serve as an extra validation to our own logic.

ericanderson · 2024-07-18T15:06:49Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkInterface.ts

@@ -50,7 +50,7 @@ export function createOsdkInterface<
  };

  if (process.env.TARGET !== "browser") {
-    Object.setPrototypeOf(interfaceHolder, OsdkCustomInspectPrototype);
+    Object.setPrototypeOf(interfaceHolder, null);


Lets revert this change for now. It only makes sense to do this in non-browser. If it rears its head again, we can come up with a better long term solution.

ericanderson · 2024-07-18T15:08:27Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkObject.ts

@@ -38,7 +39,7 @@ import type { PropertyDescriptorRecord } from "./PropertyDescriptorRecord.js";
 const objectPrototypeCache = createClientCache(
  function(client, objectDef: FetchedObjectTypeDefinition<any, any>) {
    return Object.create(
-      process.env.target !== "browser" ? OsdkCustomInspectPrototype : null,
+      process.env.target !== "browser" ? null : null,


Lets restore this one too

ericanderson · 2024-07-18T15:16:49Z

packages/client/src/util/streamutils.ts

+const OBJECT_OPEN_CHAR_CODE = 123; // '{'
+const OBJECT_CLOSE_CHAR_CODE = 125; // '}'
+
+export async function* parseStreamedResponse(


Cool with this for now as we know it works but I'm curious if there is something a bit less custom that works out of the box for browsers? Def for a follow up.

ssanjay1 added 2 commits July 1, 2024 17:09

fix response in codegen

42f2f6c

start timeseries work

d34f1f0

ssanjay1 force-pushed the ssanjay/timeseries branch from 26c668e to d34f1f0 Compare July 2, 2024 16:43

ssanjay1 changed the base branch from main to ssanjay/codegentimeseries July 2, 2024 16:44

more progress:

149cb18

Base automatically changed from ssanjay/codegentimeseries to main July 8, 2024 14:17

ssanjay1 added 7 commits July 11, 2024 11:37

Merge branch 'main' into ssanjay/timeseries

383de83

more progress

3f0008c

Merge branch 'main' into ssanjay/timeseries

836913c

get tests working

1aa83a0

rest of timeseries

d1f3cf6

fix index files

22a9d1b

remove some commented out code

6604efc

ssanjay1 marked this pull request as ready for review July 18, 2024 02:23

ssanjay1 changed the title ~~WIP Timeseries~~ Timeseries Jul 18, 2024

ssanjay1 added 3 commits July 18, 2024 09:54

self review cleanup

769aa2e

refactor duration mappping

275d376

remove unused code

d7903d4

ssanjay1 commented Jul 18, 2024

View reviewed changes

ericanderson approved these changes Jul 18, 2024

View reviewed changes

ssanjay1 added 4 commits July 20, 2024 07:53

fix merge conflicts

22087a3

review comments

18ead79

add changeset plus one minor revision

c002847

modify report

6d1237b

ssanjay1 merged commit 489d13f into main Jul 23, 2024
7 checks passed

ssanjay1 deleted the ssanjay/timeseries branch July 23, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeseries #435

Timeseries #435

ssanjay1 commented Jul 2, 2024 •

edited

Loading

ssanjay1 Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ssanjay1 Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ericanderson Jul 18, 2024

ericanderson Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ssanjay1 Jul 18, 2024

ericanderson Jul 18, 2024

ericanderson Jul 18, 2024

ericanderson Jul 18, 2024

ericanderson Jul 18, 2024

Timeseries #435

Timeseries #435

Conversation

ssanjay1 commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssanjay1 commented Jul 2, 2024 •

edited

Loading