-
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 version compatibility checks to Queries #521
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,5 @@ | ||
--- | ||
"@osdk/client": patch | ||
--- | ||
|
||
Adds version compatibility checks to Queries |
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, | ||
}); |
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: 10_000 }, async () => { | ||
const { resp } = await tsServer.sendQuickInfoRequest({ | ||
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 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 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 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)"`, | ||
); | ||
}); | ||
}); |
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); | ||
} |
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.
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?
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.
NOOP, while producing a simpler intellisense was also stripping out the mapped jsdoc. We will have to find another way possibly!