search

JsCommunity / json-rpc-peer

JSON-RPC 2 transport-agnostic library
13 stars 3 forks source link

feat: add TypeScript definitions #60

Closed maxhawkins closed 3 years ago

maxhawkins commented 4 years ago

Fixes #50

julien-f commented 4 years ago

Not being fluent in TS, I'm much more at ease merging type definitions instead of a full conversion.

@huan @alexstrats Can you please review take a look at this PR? 🙂

huan commented 4 years ago

@maxhawkins Thanks for the contribution! I'm so happy that we have more TS fans pay attention to this great JSON RPC module. 🎉

@julien-f I understand that you have not been using TS much, which means that translating to TS will make extra workload for you to maintain.

However, to convert the project to the TS will have many benefits, for example:

  1. Clean dependencies: because TS will transpile the code so we can just use the ES6/ES7 features without depends on neither lodash nor runtime
  2. Strong typing for the code to be better maintainable. as the TypeScript itself said that it's for "Scale"
  3. The typing will be natively generated which means that's all the typing is strictly passed the unit tests. (if we just add the type definition, it's hard to say it's absolutely OK without complicated settings)

So I'd like to re-raise my suggestion for converting this repo to TypeScript so that we can make sure everything is OK with the typings.

If you could consider my proposal, I would love to volunteer to make this happen and make everything automatically happen by enabling DevOps & CI/CD for our repo, so that you will need not to do the compile/publish by hand, again and again: just a git push will be all we need.

Here's one of my TypeScript project repo (7K stars) that has been set DevOps to be published to the NPM with GitHub Actions, we just need a push to GitHub and take a coffee: https://github.com/wechaty/wechaty

Looking forward to hearing from you and please feel free to let me know if you have any worries about the conversion, thanks!

huan commented 3 years ago

@julien-f Do you think that we could merge this PR so that we can start using it and continue improving it now?

maxhawkins commented 3 years ago

I don't have time to work on this right now. Feel free to make a new PR with the changes if you like.

huan commented 3 years ago

@maxhawkins I'd like to suggest that we can merge this PR first.

I believe your typing is good to go because I'm already using it in my projects. :)

julien-f commented 3 years ago

@huan Do you want me to release it?

huan commented 3 years ago

@julien-f Thanks for merging the TS definition!

Yes, please release it for us and I'll try to use it so that we can find which part should be improved later.

BTW: Would you like to build GitHub Actions so that we can release the npm module automatically? I do this for all my NPM modules, i.e. https://github.com/wechaty/wechaty/runs/1319623817.

I can help our repo to add this feature if you like to.

julien-f commented 3 years ago

@huan v0.17.0 published

Why not, I've given some permissions on this repo, please tell me if that's not enough :slightly_smiling_face:

huan commented 3 years ago

@julien-f Joined, Thanks for adding me as a collaborator!

I'll create another PR to enable DevOps soon and let you know what we need next.