Thoughts on converting this project to Typescript?

[aldy505]

I've seen #1097, #567 and #611.

The problem with maintaining 2 package: minio and @types/minio that exists on a different repository with different published version is that you won't get 100% correct typing, just because a version mismatch. You can benefit from having to hold the Typescript declaration file without going to @types/minio using the types field on package.json, or we can generating the typings for Typescript by converting this project to Typescript.

The benefit of converting this to Typescript is of course:

1. More maintainable, I believe we've all code a statically typed language, everyone knows the benefit of that
2. No more maintaining 2 separate package, both the code and typings is on 1 package
3. We can use modern Javascript syntax/function, and have backward compatible polyfills if we're properly building it with Typescript compiler, Rollup, or equivalent.

If there are any blockers or required discussion, or even there are no plan to convert it yet the members of Minio accept the idea of converting this project to Typescript by community contribution (without ghosting/ignoring the contribution), please let us know.

[prakashsvmx]

@aldy505 We can slowly migrate to TS may be on a different branch . So once we have all the tests passing and validate it, we can create a pull request to the master branch.

[trim21]

thought on rewriting internal async mechanism to promise and async iterator too? I think it fit better for typescript

code would look like this, asCallback will return a promise if cb is undefined， and using for await to collect response, so don't need to pipe here and there in most case

(this is a example to not change widely used makeRequest, makeRequest is wrapped with promise into makeRequestPromise)

  listBuckets(cb?: ResultCallback<BucketItemFromList[]>): void | Promise<BucketItemFromList[]> {
    let method = 'GET'
    return asCallbackFn(cb, async () => {
      const response = await this.makeRequestPromise({ method }, '', [200], DEFAULT_REGION)

      const body = await readAll(response)

      return xmlParsers.parseListBucket(body.toString())
    })
  }

node runtime only support for await after node10 but it won't be a problme because we will have babel to transform them.

Also sugesst to remove callback and provide promise style api only as breaking change in v8.

promise will support callback well as client.$call.then(r =>cb(null, r),cb) and it will simplify the internal implyment

[aldy505]

thought on rewriting internal async mechanism to promise and async iterator too? I think it fit better for typescript

yes, I'd love that.

Also sugesst to remove callback and provide promise style api only as breaking change in v8.

to be honest, I hate breaking changes. so, I suppose we'll have to keep the callback for a while, at least until the next major version changes.

for now, I believe that it's best to progressively migrate parts of typescript, rather than purposely rewrite and make everything better with lots of breaking change like what node-redis library did a year (or two? maybe?) ago.

perhaps @prakashsvmx might have a word about this?

then again, I'm a person who always believe on the community, if most people are okay of breaking changes for utilizing new node features, then it's fine by me.

[prakashsvmx]

We could always improve progressively without breaking changes. Because any refactoring would lead to more involved effort.

[trim21]

I have a question, where is the implement of deprecated putObjectTagging method?

I can see there are test cases for it but I can't find it's implement

https://github.com/minio/minio-js/blob/b74894efdb03b3877388f03c6132dd901546063d/src/test/unit/test.js#L952-L959

are these tests should be removed? or the catch here are actually catching TypeError undefined is not function ?

[trim21]

🤔I have done the migration, just wait PR to be reviewed and merged

[trim21]

thought on rewriting internal async mechanism to promise and async iterator too? I think it fit better for typescript

a promise async/await version of fPutObject （no api change)

https://github.com/trim21/minio-js/blob/81095dc271c7be78636afdb2d67219d300422a6c/src/main/client.ts#L1281-L1306

original

https://github.com/minio/minio-js/blob/119b8c21041b28264f793d0adf826e4bd0060316/src/main/minio.js#L1076

[aldy505]

Because #1120 is taking too long to be merged, I have a question for @prakashsvmx: who maintains the @types/minio package? Can we migrate it here? So the package.json file will be:

{
  "name": "minio",
  // ...
  "main": "./dist/main/minio.js",
  "types": "./index.d.ts", // <-- THIS
  // ...
}

So we can solve issues like #1097 easily without having to maintain 2 packages at once. It will not cause breaking change either. We can eventually deprecate the @types/minio when it's migrated here.

[trim21]

who maintains the @types/minio package?

anyone can send a PR https://github.com/DefinitelyTyped/DefinitelyTyped/commits/master/types/minio

[trim21]

by the way can maintainers take time to review my PR #1120 ? 👀 it doesn't change any code, only formatting

[aldy505]

I know anyone can send a PR, but because MinIO itself is a company, I suppose that there is someone who usually looks after it.

Sent from Proton Mail for iOS

On Mon, Apr 24, 2023 at 08:46, Trim21 @.***> wrote:

who maintains the @types/minio package?

anyone can send a PR https://github.com/DefinitelyTyped/DefinitelyTyped/commits/master/types/minio

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[trim21]

No there isn't...

[aldy505]

So sad.

Sent from Proton Mail for iOS

On Mon, Apr 24, 2023 at 08:58, Trim21 @.***> wrote:

No there isn't...

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>