sdks/ts: replace any with proper types in SDK core#2857
Conversation
|
✅ All contributors have signed the CLA. |
Replace `any` types with `unknown`, generics, and proper type constraints across the SDK. Uses Reflect.construct for safe instantiation, Object.assign for typed function objects, and generic wrappers for deserialize functions.
Myestery
force-pushed
the
fix/sdks-ts-type-improvements
branch
from
66d9fd4 to
4dd8861
Compare
| */ | ||
| export type OperationErrors<T extends Operation<any, any, any>[]> = { | ||
| [K in keyof T]: T[K] extends Operation<any, any, infer Err> ? Err : never; | ||
| export type OperationErrors<T extends Operation<unknown, unknown, unknown>[]> = { |
There was a problem hiding this comment.
This doesn't give any extra safety, but I am not totally against this change
| } | ||
|
|
||
| function assertErrorInstanceOf<T, C extends abstract new (..._: any) => any>( | ||
| function assertErrorInstanceOf<T, C extends abstract new (..._: unknown[]) => unknown>( |
There was a problem hiding this comment.
Again not much value add. It doesn't provide extra safety, but not "against" this change just because it kind of look better than previous version
There was a problem hiding this comment.
Any is not unknown. Sometimes any does makes more sense.
| } | ||
|
|
||
| function unsupportedWithHint(kind: string, hint: string): Handler<any> { | ||
| function unsupportedWithHint<K extends TsType['kind']>(kind: string, hint: string): Handler<K> { |
|
|
||
| const nameAndElementValues = serializeMultimodalToDataValue(tsValue, multiModalTypeInfo); | ||
| const nameAndElementValues = serializeMultimodalToDataValue( | ||
| tsValue as Record<string, unknown>[], |
There was a problem hiding this comment.
No extra safety, as far as I see.
|
|
||
| // type mismatch in tsValue when converting from TS to WIT | ||
| export function typeMismatchInSerialize(tsValue: any, expectedType: string): string { | ||
| export function typeMismatchInSerialize(tsValue: unknown, expectedType: string): string { |
There was a problem hiding this comment.
If you think about this, it's not unknown. its any
| type AllOptional<T extends readonly unknown[], I extends any[] = []> = T extends readonly [ | ||
| any, | ||
| type AllOptional<T extends readonly unknown[], I extends unknown[] = []> = T extends readonly [ | ||
| unknown, |
There was a problem hiding this comment.
Again no much safety introduction.
But this version is not harming. Hence approving
|
I would recommend not changing much on That said, thanks for making some parts of the code look better. |
| get?: unknown; | ||
| newPhantom?: unknown; | ||
| getPhantom?: unknown; | ||
| }; |
There was a problem hiding this comment.
This is just delaying the "guilt" :D Unless I don't understand how it can make it safe. I trust it doesn't harm though
|
Lot of conflicts due to my optimisations. Let's resolve them, but make sure the optimizations are not reverted |
…ovements # Conflicts: # sdks/ts/packages/golem-ts-sdk/src/decorators/agent.ts # sdks/ts/packages/golem-ts-sdk/src/internal/clientGeneration.ts # sdks/ts/packages/golem-ts-sdk/src/internal/mapping/values/Value.ts # sdks/ts/packages/golem-ts-sdk/src/internal/mapping/values/WitValue.ts # sdks/ts/packages/golem-ts-sdk/src/internal/mapping/values/dataValue.ts # sdks/ts/packages/golem-ts-sdk/src/internal/mapping/values/deserializer.ts # sdks/ts/packages/golem-ts-sdk/src/internal/resolvedAgent.ts
3 participants
Summary
anytypes withunknown, generics, and proper type constraints across ~15 SDK filesReflect.constructfor safe class instantiation in the decoratorObject.assignpattern for typed function objects in client generationdeserialize/deserializeDataValueto maintain public API types while usingunknowninternallyMethodKeys<T>helper type for cleanerClient<T>definitionMutableAgentStaticstype to avoid(ctor as any).get = ...patternContext
Split from #2845 based on reviewer feedback. This PR focuses only on SDK type improvements, excluding:
Test plan
tsc --noEmitpasses