Better abstractions and types #66
Conversation
There was a problem hiding this comment.
Looks really good :)
Good cleanup types with CompositeValue and ObjectLike type and Type abstract class.
Most comments are minor type changes. (all instances of object should be changed to CompositeValue) (Record<string, unknown> should be replaced with ObjectLike where possible)
Only issue I have is re: usage of isComposite.
I think its a useful method for consumers, but internal to the library, IMO its not necessary and should be replaced with type coersion as necessary.
SSZ has several invariants that don't need to be checked at runtime (basic types are always fixed-length, types are always either basic or composite)
| @@ -88,7 +88,7 @@ export class BasicArrayStructuralHandler<T extends ArrayLike<unknown>> extends S | |||
| } | |||
| } | |||
|
|
|||
| export class CompositeArrayStructuralHandler<T extends ArrayLike<object>> extends StructuralHandler<T> { | |||
| export class CompositeArrayStructuralHandler<T extends ArrayLike<ObjectLike>> extends StructuralHandler<T> { | |||
There was a problem hiding this comment.
I don't think this is right?
Seems like some of the changes in this PR have object being changed to ObjectLike.
Should be CompositeValue.
There was a problem hiding this comment.
If I put CompositeValue, that allows array element to be another array, which is perfectly fine. But our types doesn't reflect that anywhere so there is huge number of type errors. It's kinda hard because wherever I change type I cause chain reaction.
Not sure how to proceed. Maybe we can in separate PRs fix types from bottom up?
| fieldType.structural.assertValidValue((value as T)[fieldName]); | ||
| fieldType.assertValidValue(value[fieldName]); | ||
| } | ||
| if (fieldType.isComposite()) { |
There was a problem hiding this comment.
else if?
Is this changed to fix the type? Should the ts-ignore be removed then? If the ts-ignore can't be removed, the code shouldn't be changed.
There was a problem hiding this comment.
If you wanna add another like in this code block, it will be typesafe. I have no idea how to fix wierd error with that assertValidValue. Tried every combination. I guess you had same one since it was ignored before 😄
| newValue[fieldName as keyof T] = fieldType.structural.clone(value[fieldName]); | ||
| newValue[fieldName as keyof T] = fieldType.clone(value[fieldName]) as T[keyof T]; | ||
| } | ||
| if (fieldType.isComposite()) { |
There was a problem hiding this comment.
Because typescript is stupid. It cannot (or at least I didn't find a way) to infer if isBasic is false that type is composite. Basically in else statement fieldType is of Type which causes type errors,
That said when going trough code. It was a lot easier to understand code with usage of isComposite to I decided to leave it on. We can make it protected so end users don't see it.
| @@ -0,0 +1 @@ | |||
| export type ArrayElement<T> = T extends ArrayLike<infer U> ? U : T; | |||
There was a problem hiding this comment.
I think it is. I was trying to bring even more type safety to infer element type from List and Vector. Will check
|
|
||
| export class ContainerStructuralHandler<T extends ObjectLike> extends StructuralHandler<T> { | ||
| export class ContainerStructuralHandler<T extends Record<string, unknown>> extends StructuralHandler<T> { |
There was a problem hiding this comment.
that screws everything since it ObjectLike allows empty object without keys which casues keyof to stop working
|
Good ideas in this PR! But I would close for now since the codebase has changed substantially since #82. If this effort is gonna be pursued in the future better start from a new branch from current master |
Still wip but provides basic frame to start sharing more code and have better typesafety when dealing with ssz.
TODO: