-
Notifications
You must be signed in to change notification settings - Fork 177
sdks/ts: enforce no-explicit-any and replace any with proper types #2845
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
Changes from all commits
eb18ce0
7991d57
d4ef273
3135b06
ac3cc28
d3c2a7f
e9906cd
ff4b2b0
ed700eb
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 |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| # Logs | ||
| logs | ||
| storybook-static | ||
| *.log | ||
| npm-debug.log* | ||
| yarn-debug.log* | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ export type AroundInvokeHook = { | |
| beforeInvoke: (request: AgentInvocationRequest) => Promise<void>; | ||
| afterInvoke: ( | ||
| request: AgentInvocationRequest, | ||
| result: JsonResult<AgentInvocationResult, any>, | ||
| result: JsonResult<AgentInvocationResult, unknown>, | ||
| ) => Promise<void>; | ||
| }; | ||
|
|
||
|
|
@@ -41,6 +41,52 @@ export type EnvironmentName = string; | |
| export type AgentTypeName = string; | ||
| export type IdempotencyKey = string; | ||
|
|
||
| export type Timestamp = string; // ISO 8601 | ||
|
|
||
| export type WorkerStatus = | ||
|
Contributor
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. These are defined in WIT so the binding generator (of wasm-rquickjs) should generate good types for them. If not, can we improve there? To decide that please explain why you defined new ones
Contributor
Author
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 searched for worker definitions but didn't find one.And the old implementation just had worker name
Contributor
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 original "mapping" is very lightweight intenionally, and just for looking up agent ids for completion from CLI output. we do have plans (and will soon have tickets) around exploring more typed integration with the CLI, so i see no point on having these here for now, especially that we currently keep changing APIs / CLI interfaces as part of the more agent focused APIs |
||
| | 'Running' | ||
| | 'Idle' | ||
| | 'Suspended' | ||
| | 'Interrupted' | ||
| | 'Retrying' | ||
| | 'Failed' | ||
| | 'Exited'; | ||
|
|
||
| export type UpdateRecord = | ||
| | { type: 'PendingUpdate'; timestamp: Timestamp; targetRevision: number } | ||
| | { type: 'SuccessfulUpdate'; timestamp: Timestamp; targetRevision: number } | ||
| | { | ||
| type: 'FailedUpdate'; | ||
| timestamp: Timestamp; | ||
| targetRevision: number; | ||
| details?: string; | ||
| }; | ||
|
|
||
| export type WorkerResourceDescription = { | ||
| createdAt: Timestamp; | ||
| resourceOwner: string; | ||
| resourceName: string; | ||
| }; | ||
|
|
||
| export type Worker = { | ||
| componentName: string; | ||
| workerName: string; | ||
| createdBy: string; | ||
| environmentId: string; | ||
| env: Record<string, string>; | ||
| configVars: Record<string, string>; | ||
| status: WorkerStatus; | ||
| componentRevision: number; | ||
| retryCount: number; | ||
| pendingInvocationCount: number; | ||
| updates: UpdateRecord[]; | ||
| createdAt: Timestamp; | ||
| lastError?: string; | ||
| componentSize: number; | ||
| totalLinearMemorySize: number; | ||
| exportedResourceInstances: Record<string, WorkerResourceDescription>; | ||
| }; | ||
|
|
||
| export type DataValue = | ||
| | { type: 'Tuple'; elements: ElementValue[] } | ||
| | { type: 'Multimodal'; elements: NamedElementValue[] }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,10 +33,14 @@ export type ReplCliFlags = { | |
| streamLogs: boolean; | ||
| }; | ||
|
|
||
| export type AgentConfig = { | ||
| export type AgentModule = Record<string, unknown> & { | ||
|
Contributor
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 bridge SDK package is not really a record of string, it is a package, the plumbing here does not really change type safety compared to before. |
||
| configure: ConfigureClient; | ||
| }; | ||
|
|
||
| export type AgentConfig<T extends AgentModule = AgentModule> = { | ||
| clientPackageName: string; | ||
| clientPackageImportedName: string; | ||
| package: any; | ||
| package: T; | ||
| }; | ||
|
|
||
| export type ConfigureClient = (config: base.Configuration) => void; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ import { | |
| cliCommandsConfigFromBaseConfig, | ||
| clientConfigFromEnv, | ||
| Config, | ||
| ConfigureClient, | ||
| loadReplCliFlags, | ||
| ReplCliFlags, | ||
| } from './config'; | ||
|
|
@@ -25,13 +24,13 @@ import { LanguageService } from './language-service'; | |
| import pc from 'picocolors'; | ||
| import repl, { type REPLEval } from 'node:repl'; | ||
| import process from 'node:process'; | ||
| import { AsyncCompleter } from 'readline'; | ||
| import { Completer, AsyncCompleter } from 'readline'; | ||
| import { PassThrough } from 'node:stream'; | ||
| import { ts } from 'ts-morph'; | ||
| import { flushStdIO, setOutput, writeln } from './process'; | ||
| import { formatAsTable, formatEvalError, logSnippetInfo } from './format'; | ||
| import * as base from './base'; | ||
| import { AgentInvocationRequest, AgentInvocationResult, JsonResult } from './base'; | ||
| import { AgentInvocationRequest } from './base'; | ||
|
|
||
| const MAX_COMPLETION_ENTRIES = 50; | ||
|
|
||
|
|
@@ -61,11 +60,7 @@ export class Repl { | |
| } | ||
| }, | ||
|
|
||
| async afterInvoke( | ||
| request: AgentInvocationRequest, | ||
| result: JsonResult<AgentInvocationResult, any>, | ||
| ): Promise<void> { | ||
| void result; | ||
| async afterInvoke(request: AgentInvocationRequest): Promise<void> { | ||
| await cli.stopAgentStream(request); | ||
| }, | ||
| }; | ||
|
|
@@ -80,11 +75,13 @@ export class Repl { | |
|
|
||
| private newBaseReplServer(options?: { | ||
| input?: NodeJS.ReadableStream; | ||
| output?: NodeJS.WritableStream; | ||
| output?: NodeJS.WriteStream; | ||
| terminal?: boolean; | ||
| eval?: REPLEval; | ||
| completer?: Completer | AsyncCompleter; | ||
| }): repl.REPLServer { | ||
| const output = options?.output ?? process.stdout; | ||
| const terminal = options?.terminal ?? Boolean((output as any).isTTY); | ||
| const terminal = options?.terminal ?? Boolean(output.isTTY); | ||
| const prompt = this.replCliFlags.script | ||
| ? '' | ||
| : `${pc.cyan('golem-ts-repl')}` + | ||
|
|
@@ -101,13 +98,13 @@ export class Repl { | |
| preview: false, | ||
| ignoreUndefined: true, | ||
| prompt, | ||
| eval: options?.eval, | ||
| completer: options?.completer, | ||
| }); | ||
| } | ||
|
|
||
| private async setupRepl(replServer: repl.REPLServer): Promise<void> { | ||
| await this.setupReplHistory(replServer); | ||
| this.setupReplEval(replServer); | ||
| this.setupReplCompleter(replServer); | ||
| this.setupReplContext(replServer); | ||
| this.setupReplCommands(replServer); | ||
| } | ||
|
|
@@ -124,13 +121,12 @@ export class Repl { | |
| }); | ||
| } | ||
|
|
||
| private setupReplEval(replServer: repl.REPLServer): repl.REPLEval { | ||
| const tsxEval = replServer.eval; | ||
| private createCustomEval(tsxEval: REPLEval): REPLEval { | ||
| const languageService = this.getLanguageService(); | ||
| const replCliFlags = this.replCliFlags; | ||
| const getOverrideSnippet = () => this.overrideSnippetForNextEval; | ||
|
|
||
| const customEval: REPLEval = function (code, context, filename, callback) { | ||
| return function (this: repl.REPLServer, code, context, filename, callback) { | ||
| const evalCode = (code: string) => { | ||
| tsxEval.call(this, code, context, filename, (err, result) => { | ||
| if (!err) { | ||
|
|
@@ -179,16 +175,12 @@ export class Repl { | |
| callback(null, undefined); | ||
| } | ||
| }; | ||
| (replServer.eval as any) = customEval; | ||
|
|
||
| return tsxEval; | ||
| } | ||
|
|
||
| private setupReplCompleter(replServer: repl.REPLServer) { | ||
| const nodeCompleter = replServer.completer; | ||
| private createCustomCompleter(nodeCompleter: Completer | AsyncCompleter): AsyncCompleter { | ||
| const languageService = this.getLanguageService(); | ||
| const cli = this.cli; | ||
| const customCompleter: AsyncCompleter = function (line, callback) { | ||
| return function (line, callback) { | ||
| if (line.trimStart().startsWith('.')) { | ||
| cli | ||
| .complete(line) | ||
|
|
@@ -216,7 +208,6 @@ export class Repl { | |
| } | ||
| } | ||
| }; | ||
| (replServer.completer as any) = customCompleter; | ||
| } | ||
|
|
||
| private setupReplContext(replServer: repl.REPLServer) { | ||
|
|
@@ -227,7 +218,7 @@ export class Repl { | |
| const context = replServer.context; | ||
| for (let agentTypeName in this.config.agents) { | ||
| const agentConfig = this.config.agents[agentTypeName]; | ||
| let configure = agentConfig.package.configure as ConfigureClient; | ||
| let configure = agentConfig.package.configure; | ||
| configure(this.clientConfig); | ||
| context[agentTypeName] = agentConfig.package[agentTypeName]; | ||
| context[agentConfig.clientPackageImportedName] = agentConfig.package; | ||
|
|
@@ -346,13 +337,25 @@ export class Repl { | |
| setOutput('stdout'); | ||
|
|
||
| const script = this.replCliFlags.script; | ||
|
|
||
| // We need to create a temporary repl server to get the default eval and completer | ||
|
Contributor
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 pactching of the repl was intenionally used the actual instance. (And there is another patching in place before, that is created by tsx, and we follow the way). Not sure it this breaks anything, but there is no point in changing the current solution. |
||
| const tempRepl = repl.start({ input: new PassThrough(), output: new PassThrough() }); | ||
| const customEval = this.createCustomEval(tempRepl.eval); | ||
| const customCompleter = this.createCustomCompleter(tempRepl.completer); | ||
| tempRepl.close(); | ||
|
|
||
| const replServer = script | ||
| ? this.newBaseReplServer({ | ||
| input: new PassThrough(), | ||
| output: process.stdout, | ||
| terminal: false, | ||
| eval: customEval, | ||
| completer: customCompleter, | ||
| }) | ||
| : this.newBaseReplServer(); | ||
| : this.newBaseReplServer({ | ||
| eval: customEval, | ||
| completer: customCompleter, | ||
| }); | ||
|
|
||
| await this.setupRepl(replServer); | ||
|
|
||
|
|
||
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.
this format look off (also this code will be soon moved to the sdk)