search

terascope / file-assets

Teraslice processors for working with data stored in files on disk, S3 or HDFS.
MIT License
1 stars 2 forks source link

Type error when updating file-asset-apis #1047

Open busma13 opened 3 weeks ago

busma13 commented 3 weeks ago

When updating file-asset-apis to v 1.0.0 in teraslice/terafoundation there are 2 type errors:

packages/teraslice/src/lib/storage/backends/s3_store.ts:79:53 - error TS2339: Property 'BucketAlreadyExists' does not exist on type 'typeof import("/Users/<username>/WORKSPACE/teraslice/node_modules/@terascope/file-asset-apis/dist/src/s3/client-types/client-response")'.

79                 if (err instanceof S3ClientResponse.BucketAlreadyExists) {
                                                       ~~~~~~~~~~~~~~~~~~~

packages/teraslice/src/lib/storage/backends/s3_store.ts:128:49 - error TS2339: Property 'NoSuchKey' does not exist on type 'typeof import("/Users/<username>/WORKSPACE/teraslice/node_modules/@terascope/file-asset-apis/dist/src/s3/client-types/client-response")'.

128             if (err instanceof S3ClientResponse.NoSuchKey) {

I can navigate to client-response.d.ts and the types are present.

sotojn commented 3 weeks ago

instanceof works by checking the object's prototype chain against the prototype of the specified constructor function. This means it needs access to the actual constructor function of the class.

The issue is that the last update to v1.0.0 in file-asset-apis changed these exports to just export the type instead of the actual class.

Another thing I learned about types is that they do not exist at runtime, so they are not properties of the imported object. Meaning you can only use types for checking and defining structure but cannot access them as actual values or properties at runtime. The way we're using them here is trying to access the imported object S3ClientResponse then looking in the object for a NoSuchKey type property, but again the don't exist at runtime. Hence the error saying the property doesn't exist.

Solution: Update file-asset-apis to export the full classes so we can use them in teraslice. This could be a separate export called client-classes that we would use instead. Or we could just rename client-types to client-classes and have the dev using file-asset-apis to import the types using import types

https://github.com/terascope/file-assets/blob/5922e31fb3d9706b64914a225245f8caa70e79b3/packages/file-asset-apis/src/s3/client-types/client-response.ts#L6

jsnoble commented 3 weeks ago

Yes, types only exist for type checking and do not exist in runtime unless its an enum, that translates to an actual value. If you need the actual things, then all you need to do is change export type { to export {.

I agree with the notion of changing the name of the top level directory as client-types make you think they are all types. Maybe just change it to native-client-helpers or the like, make sure to change client-params as well as client-repsonse to remove the type