Chore/typescript conversion/27 #33
Conversation
| switch (type) { | ||
| case "BOOLEAN": | ||
| return encodeValues_BOOLEAN(values as Array<boolean>); |
There was a problem hiding this comment.
I wonder if we could remove the as ... on each of these and just have encodeValues do the work. Thoughts?
There was a problem hiding this comment.
I tried removing the as ... but have run into issues on typescript not liking that very much. Similar issue to Shannons comment below
There was a problem hiding this comment.
That works. We are saying it is that type, so I'm ok with it.
lib/codec/rle.ts
Outdated
| @@ -42,7 +43,7 @@ exports.encodeValues = function(type, values, opts) { | |||
| case 'BOOLEAN': | |||
| case 'INT32': | |||
| case 'INT64': | |||
| values = values.map((x) => parseInt(x, 10)); | |||
| values = values.map((x) => parseInt(x as unknown as string, 10)); // We should fix unknown as string | |||
There was a problem hiding this comment.
I think the correct fix might be to set vales: Array at the top and have a helper function that converts from unknown to a parsed int.
Also I thought we fixed all the places where we have Int64 to use bigint instead.
There was a problem hiding this comment.
Implemented helper function to fix. Not sure about the Int64 but I do know that the toPrimitives that I worked on return number if the value given is number but bigint otherwise if thats what you were referencing
lib/compression.ts
Outdated
| deflate: Function, | ||
| inflate: Function |
There was a problem hiding this comment.
I wonder if we can get more specific than just Function here
| deflate: Function, | |
| inflate: Function | |
| deflate: <T>(value: Buffer | string | ArrayBuffer | Uint8Array) => T; | |
| inflate: <T>(value: T) => Buffer | string | ArrayBuffer | Uint8Array; |
There was a problem hiding this comment.
@wilwade I worked with @waylandli on this and it's not as simple as it might seem, because the union type does not work here, and it doesn't even work to declare the functions as types and use a union of those. So it's going to need more of a refactor than at first blush. I suggest we punt to a later PR.
There was a problem hiding this comment.
I'm good with that punt. Sounds like the interface there is a bit problematic and might take some code changes (beyond the conversion)
lib/codec/plain.ts
Outdated
| }; | ||
|
|
||
| export const decodeValues = function ( | ||
| type: string, |
There was a problem hiding this comment.
| type: string, | |
| type: ValidValueTypes | string, |
| switch (type) { | ||
| case "BOOLEAN": | ||
| return encodeValues_BOOLEAN(values as Array<boolean>); |
There was a problem hiding this comment.
That works. We are saying it is that type, so I'm ok with it.
Problem
[Github Issues #27 ]
Converted the files listed in the issues into typescript without using any
Change summary:
Steps to Verify:
Running all the tests should still work