Skip to content
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 version compatibility checks to Queries #521

Merged
merged 2 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/moody-parrots-sing.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@osdk/client": patch
---

Adds version compatibility checks to Queries
2 changes: 1 addition & 1 deletion etc/client.api.report.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ export interface PropertyValueWireToClient {
// Warning: (ae-forgotten-export) The symbol "OptionalQueryParams" needs to be exported by the entry point index.d.ts
//
// @public (undocumented)
export type QueryParameterType<T extends Record<any, QueryDataTypeDefinition<any, any>>> = NOOP<PartialByNotStrict<NotOptionalParams_2<T>, OptionalQueryParams<T>>>;
export type QueryParameterType<T extends Record<any, QueryDataTypeDefinition<any, any>>> = PartialByNotStrict<NotOptionalParams_2<T>, OptionalQueryParams<T>>;

// @public (undocumented)
export type QueryReturnType<T extends QueryDataTypeDefinition<any, any>> = T extends ObjectQueryDataType<any, infer TTargetType> ? OsdkBase<TTargetType> : T extends ObjectSetQueryDataType<any, infer TTargetType> ? ObjectSet<TTargetType> : T["type"] extends keyof DataValueWireToClient ? DataValueWireToClient[T["type"]] : never;
Expand Down
2 changes: 1 addition & 1 deletion packages/client.api/src/queries/Queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export type QuerySignatureFromDef<T extends QueryDefinition<any, any>> =

export type QueryParameterType<
T extends Record<any, QueryDataTypeDefinition<any, any>>,
> = NOOP<PartialByNotStrict<NotOptionalParams<T>, OptionalQueryParams<T>>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the NOOP? I remember when you explained this to me, it was to help expand the type out right,e.g. when you hovered over it, or am I misremembering?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOOP, while producing a simpler intellisense was also stripping out the mapped jsdoc. We will have to find another way possibly!

> = PartialByNotStrict<NotOptionalParams<T>, OptionalQueryParams<T>>;

export type QueryReturnType<T extends QueryDataTypeDefinition<any, any>> =
T extends ObjectQueryDataType<any, infer TTargetType> ? OsdkBase<TTargetType>
Expand Down
5 changes: 5 additions & 0 deletions packages/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"conjure-lite": "^0.4.4",
"fast-deep-equal": "^3.1.3",
"fetch-retry": "^6.0.0",
"find-up": "7.0.0",
"isomorphic-ws": "^5.0.0",
"tiny-invariant": "^1.3.1",
"type-fest": "^4.18.2",
Expand All @@ -69,9 +70,13 @@
"@osdk/shared.test": "workspace:~",
"@types/geojson": "^7946.0.14",
"@types/ws": "^8.5.11",
"execa": "^9.3.0",
"jest-extended": "^4.0.2",
"msw": "^2.3.4",
"p-defer": "^4.0.1",
"p-event": "^6.0.1",
"p-locate": "^6.0.0",
"p-map": "^7.0.2",
"p-state": "^2.0.1",
"pino": "^9.1.0",
"pino-pretty": "^11.2.1",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* 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.
*/

// WARNING!
// WARNING!
// This file is used for tests that check intellisense. Editing this file by hand will likely
// break tests that have hard coded line numbers and line offsets.

import { queryAcceptsObject } from "@osdk/client.test.ontology";
import type { Client } from "../Client.js";

const client: Client = {} as any;
client(queryAcceptsObject)({
object: undefined as any,
});
111 changes: 111 additions & 0 deletions packages/client/src/intellisense.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
* Copyright 2023 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 { findUpSync } from "find-up";
import * as path from "node:path";
import type { Logger } from "pino";
import invariant from "tiny-invariant";
import * as ts from "typescript";
import {
afterEach,
beforeAll,
beforeEach,
describe,
expect,
it,
vi,
} from "vitest";
import type { TsServer } from "./tsserver.js";
import { startTsServer } from "./tsserver.js";

// it needs to be hoisted because its referenced from our mocked WebSocket
// which must be hoisted to work
const rootLogger = await vi.hoisted(async (): Promise<Logger> => {
const pino = (await import("pino")).pino;
const pinoPretty = await import("pino-pretty");
const { EventEmitter } = await import("node:events");
class PinoConsoleLogDestination extends EventEmitter {
write(a: string) {
// remove trailing newline since console.log adds one
if (a.at(-1) === "\n") a = a.slice(0, -1);

// This lets the test framework aggregate the logs per test, whereas direct to stdout does not
console.log(a);
}
}
return pino(
{ level: "info" },
(pinoPretty.build)({
sync: true,
timestampKey: undefined,
errorLikeObjectKeys: ["error", "err", "exception"],
errorProps: "stack,cause,properties",
ignore: "time,hostname,pid",
destination: new PinoConsoleLogDestination(),
}),
);
});

describe("intellisense", () => {
let packagesDir: string;
let clientPackagePath: string;

beforeAll(() => {
const clientsPackageJson = findUpSync("package.json", {
cwd: import.meta.url,
});
invariant(clientsPackageJson != null);
packagesDir = path.join(
path.dirname(clientsPackageJson),
"..",
);

clientPackagePath = path.join(packagesDir, "client");
});

let tsServer: TsServer;
let intellisenseFilePath: string;

beforeEach(async (a) => {
intellisenseFilePath = path.join(
clientPackagePath,
"src",
"intellisense.test.helpers",
`${a.task.name}.ts`,
);

expect(ts.sys.fileExists(intellisenseFilePath)).toBeTruthy();

tsServer = await startTsServer(rootLogger);
await tsServer.sendOpenRequest({ file: intellisenseFilePath });
});

afterEach(async () => {
tsServer.stop();
tsServer = undefined as any;
});

it("callsQueryAcceptsObject", { timeout: 20_000 }, async () => {
const { resp } = await tsServer.sendQuickInfoRequest({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool, so how do you imagine we'd expand these types of tests? I'm just trying to understand the scope of things we'd be looking for

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how vscode does stuff basically so anything you can visually see in vscode we can test this way

file: intellisenseFilePath,
line: 27,
offset: 6,
});
expect(resp.body?.documentation).toMatchInlineSnapshot(
`"(no ontology metadata)"`,
);
});
});
200 changes: 200 additions & 0 deletions packages/client/src/tsserver.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
/*
* 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 { Subprocess } from "execa";
import { execaNode } from "execa";
import { findUpMultiple } from "find-up";
import { EventEmitter } from "node:events";
import * as fs from "node:fs/promises";
import * as path from "node:path";
import pLocate from "p-locate";
import pMap from "p-map";
import type { Logger } from "pino";
import invariant from "tiny-invariant";
import { server as s } from "typescript";

class TsServerImpl extends EventEmitter<{
exit: [];
}> {
#tsServerPath: string;
#nextSeq = 1;
#subprocess: Subprocess<{ ipc: true; serialization: "json" }> | undefined;
#logger: Logger;

constructor(tsServerPath: string, logger: Logger) {
super();
this.#tsServerPath = tsServerPath;
this.#logger = logger;
}

get subprocess() {
return this.#subprocess;
}

async start() {
this.#subprocess = execaNode({
ipc: true,
serialization: "json",
})`${this.#tsServerPath} --useNodeIpc`;

if (this.#logger.isLevelEnabled("trace")) {
this.#subprocess.on("message", (req) => {
this.#logger.trace({ req }, "message received");
});
}

this.#subprocess.on("exit", () => {
this.#logger.info("tsserver exited");
this.emit("exit");
});
return this;
}

stop() {
if (this.#subprocess?.connected) {
this.#subprocess?.disconnect();
}
}

async getOneMessage<X>(filter?: (m: unknown) => m is X): Promise<X> {
return await this.subprocess!.getOneMessage({ filter }) as X;
}

#requestFactory =
<T extends s.protocol.Request, X extends s.protocol.Response = never>(
command: T["command"],
isResponse?: (m: unknown) => m is X,
) =>
async (args: T["arguments"]): Promise<{ req: T; resp: X }> => {
return await this.#makeRequest<T, X>(command, args, isResponse);
};

sendOpenRequest = this.#requestFactory<s.protocol.OpenRequest>(
s.protocol.CommandTypes.Open,
);

sendQuickInfoRequest = this.#requestFactory<
s.protocol.QuickInfoRequest,
s.protocol.QuickInfoResponse
>(
s.protocol.CommandTypes.Quickinfo,
isQuickInfoResponse,
);

async #makeRequest<
T extends s.protocol.Request,
X extends s.protocol.Response = never,
>(
command: T["command"],
args: T["arguments"],
isResponse?: (m: unknown) => m is X,
): Promise<{ req: T; resp: X }> {
const seq = this.#nextSeq++;
const req: T = {
type: "request",
command,
arguments: args,
seq,
} as T;
this.#logger.trace({ req }, "requesting");

await this.#subprocess?.sendMessage(req as any);

if (isResponse) {
return {
req,
resp: await this.#subprocess?.getOneMessage({
filter: isResponse,
}) as unknown as X,
};
}
return { req, resp: undefined as unknown as X };
}
}

export type TsServer = Omit<
TsServerImpl,
Exclude<
keyof EventEmitter,
| "on"
| "addListener"
| "off"
| "once"
| "removeListener"
| "removeAllListeners"
>
>;

export async function startTsServer(logger: Logger): Promise<TsServer> {
const tsServerPath = await getTsServerPath();
invariant(tsServerPath != null);

return new TsServerImpl(tsServerPath, logger).start();
}

async function getTsServerPath() {
const nodeModuleDirs = await findUpMultiple("node_modules", {
cwd: import.meta.url,
type: "directory",
});
const possibleTsServerPaths = await pMap(
nodeModuleDirs,
(dir) => path.join(dir, "typescript", "lib", "tsserver.js"),
);

const tsServerPath = await pLocate(
["no", ...possibleTsServerPaths],
async (dir) => {
try {
const c = await fs.stat(
dir,
);
return c.isFile();
} catch (e) {
return false;
}
},
);
return tsServerPath;
}

export function isEvent(m: unknown): m is s.protocol.Event {
return !!(m && typeof m === "object" && "type" in m
&& m.type === "event");
}

export function isResponse(m: unknown): m is s.protocol.Response {
return !!(m && typeof m === "object" && "type" in m
&& m.type === "response");
}

export function isProjectLoadingStart(
m: unknown,
): m is s.protocol.ProjectLoadingStartEvent {
return isEvent(m) && m.event === "projectLoadingStart";
}
export function isProjectLoadingEnd(
m: unknown,
): m is s.protocol.ProjectLoadingStartEvent {
return isEvent(m) && m.event === "projectLoadingFinish";
}
export function isQuickInfoResponse(
m: unknown,
requestSeq?: number,
): m is s.protocol.QuickInfoResponse {
return isResponse(m) && m.command === s.protocol.CommandTypes.Quickinfo
&& (requestSeq == null || m.request_seq === requestSeq);
}
Loading