sdks/ts: enforce no-explicit-any and replace any with proper types#2845
sdks/ts: enforce no-explicit-any and replace any with proper types#2845
Conversation
Add @typescript-eslint/no-explicit-any: 'error' to eslint configs for golem-ts-sdk, golem-ts-repl, golem-ts-typegen, and golem-ts-types-core to catch explicit any usage at lint time.
- types-core: use unknown in type definitions and metadata - typegen: use ClassMetadataJSON/MethodMetadataJSON instead of Record<string, any> - repl: use unknown in hooks/config, add AgentModule type with generic AgentConfig<T>, use TypeChecker API instead of private .getSignature(), remove unused imports - cli/ts_bridge: use unknown in base types
…alization pipeline Type safety improvements: - Replace any with unknown across serializer, deserializer, dataValue, errors, Value, WitValue, and other SDK source files - Add generic type parameters to deserialization pipeline: deserialize<T>, toTsValue<T>, deserializeDataValue<T>, deserializeRpcResult<T> allowing callers to avoid manual casts - Add generic to getValFieldFromTaggedObject<T> with unified return type - Use any[] with eslint-disable only where required by TypeScript contravariance (legacy decorator signatures, Client<T> type filter) Bug fixes: - Fix swapped args in missingObjectKey() call in serializer - Fix handleObjectMatch null check (was comparing to string literal) - Add null guards in binary/text reference serialization - Use explicit BigInt() conversion for u64 instead of unsafe cast
- Add eslint-disable for intentional any usage in invalid agent test fixtures (testing that any param/return types are rejected) - Use discriminated union narrowing in type mapping tests - Add explicit Client<T> typing in validAgents - Refactor non-BaseAgent decorator test to use direct function call (TypeScript now rejects @agent() on non-BaseAgent at compile time) - Replace any with unknown in test helpers and mocks
…fety - Replace any[] constructor constraints with never[] in BaseAgent static methods and decorator signature - Extract AgentClass type to replace generic constructor constraints in clientGeneration - Extract MethodKeys helper type for Client<T>, removing any from function type filter - Use MutableAgentStatics type instead of Record cast in decorator - Use Reflect.construct instead of new ctor() for constructor call - Use Object.assign instead of cast for RemoteMethod proxy creation - Replace placeholder WitNode cast with valid prim-bool node - Use Reflect.get helper for dynamic property access in resolvedAgent
Add types/globals.d.ts declaring currentAgentId on globalThis so test
files can use globalThis.currentAgentId directly instead of casting
through (globalThis as unknown as { currentAgentId: string }).
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement. Please read the CLA and post the comment below to sign. I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
# Conflicts: # sdks/ts/packages/golem-ts-repl/src/base.ts # sdks/ts/packages/golem-ts-sdk/src/internal/clientGeneration.ts # sdks/ts/packages/golem-ts-sdk/tests/http.test.ts
|
|
||
| export type Timestamp = string; // ISO 8601 | ||
|
|
||
| export type WorkerStatus = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I searched for worker definitions but didn't find one.And the old implementation just had worker name
There was a problem hiding this comment.
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
| }; | ||
| afterInvoke: (request: AgentInvocationRequest, result: JsonResult<AgentInvocationResult, unknown>) => Promise<void>; | ||
| } | ||
| ; |
There was a problem hiding this comment.
this format look off (also this code will be soon moved to the sdk)
| }; | ||
|
|
||
| export type AgentConfig = { | ||
| export type AgentModule = Record<string, unknown> & { |
There was a problem hiding this comment.
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.
|
|
||
| const script = this.replCliFlags.script; | ||
|
|
||
| // We need to create a temporary repl server to get the default eval and completer |
There was a problem hiding this comment.
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.
|
General comment: while moving away from |
|
|
||
| // These are not separate parameters, but a single parameter of multimodal type | ||
| const multiModalValue: Either.Either<any[], string> = Either.all( | ||
| const multiModalValue: Either.Either<unknown[], string> = Either.all( |
There was a problem hiding this comment.
This is a generic comment.
There are a few subteilities playing against this to introduce this lint. Firstly I didn't introduce it intentionally, when it comes to TS SDK. The reason is the risk of any and the use of any are both needed to be taken by SDK. We might be able to reduce it but not for all. So while there are places where we can legitimately replace it with unknown (because unknown can be definitely a safer option in many cases since it forces narrowing before use) , many of these anys are legitimate logically.
There was a problem hiding this comment.
Legitimate because, conceptually I should be able to do anything with this unknown any type in certain places at least. Or in fact, in many places.. I should be able to invoke a function on any. Should I narrow it? May be. May be not.
I am yet to review the full PR, but just not convinced with the any hate. (even if I like proper types)
There was a problem hiding this comment.
I would like to write more, but then it become very verbose. But if you could trust, the use of any in a lot of places in our TS SDK is actually legitimate.
| return _deserialize(value, analysedType) as T; | ||
| } | ||
|
|
||
| function _deserialize(value: Value, analysedType: AnalysedType): unknown { |
There was a problem hiding this comment.
Why this redirection? Here T=unknown and you are returning unknown and once deserialization is done, that T is not legitimately used? So conceptually its any
| } | ||
|
|
||
| function buildTree(node: WitNode, nodes: WitNode[]): Value { | ||
| let tag = node.tag; |
| tag: 'placeholder', | ||
| val: undefined, | ||
| } as any); | ||
| tag: 'prim-bool', |
There was a problem hiding this comment.
I think this is honestly too many changes, that questions what's the final outcome for golem? Is it fixing any bug? or cleaning up?
If there are bug fixes in this PR, can we raise the bug fix ticket separately and fix only that?
Because, apart from that, I don't think this PR brings in better design of the code (which is in our backlog for value mapping) or readability.
So kindly raise any bugs you find, and fix only them as separate PR. If the intend is better design and readability , let's raise a ticket separately for that, and describe the design changes in PR description. Like this one I did: #2608
that PR improved a lot about schema discovery within SDK.
|
Closing this in favor of three focused PRs based on review feedback:
Dropped from scope (per reviewer discussion): REPL types, ESLint config, lock file, |
Summary
@typescript-eslint/no-explicit-any: 'error'ESLint rule to all 4 TS SDK packages (sdk, repl, typegen, types-core)anywith proper types (unknown, generics, specific interfaces) across the entire TS SDK codebasedeserialize<T>,toTsValue<T>,deserializeDataValue<T>,deserializeRpcResult<T>) so callers can avoid manual castsAgentClass,MethodKeys,MutableAgentStaticshelper types to eliminateeslint-disablecomments andas unknown ascastsmissingObjectKeyargs, incorrecthandleObjectMatchnull check, missing null guards in reference serializationtypes/globals.d.tsfor typedglobalThis.currentAgentIdin testsBug fixes
missingObjectKey('tag', tsValue)— arguments were previously swapped, producing wrong error messageshandleObjectMatch— was comparing object to string literal'interface'instead of checking for nullserializeTsValueToBinaryReference/serializeTsValueToTextReference— added null guards (typeof null === 'object'was passing the guard)