ruimarinho / bitcoin-core

A modern Bitcoin Core REST and RPC client.
477 stars 186 forks source link

working on typescript definition file #65

Closed joemphilips closed 5 years ago

joemphilips commented 6 years ago

This is WIP PR, Please don't merge yet.

I'm working on *.d.ts file for this package. refs: #59 #25 I suppose current practice for managing type definition for npm packages is to include *.d.ts file into the same repository (instead of submitting to DefinitlyTyped). So this PR. Any suggestions are welcome! Note: currently it is only for Promise based API, and not for callbacks.

joemphilips commented 6 years ago

For now, I will ignore methods for version 0.16.0 (i.e. savemempool and rescanblockchain )

joemphilips commented 6 years ago

two more things.

joemphilips commented 6 years ago

Done annotation for RPC methods. I think it can be merged now. I'm not sure how to test these files right now. although I will look for the way to do it, it may take some time.

ruimarinho commented 6 years ago

@joemphilips this is great work, thank you! The handbook says the following:

There are two main ways you can publish your declaration files to npm:

  1. bundling with your npm package, or
  2. publishing to the @types organization on npm.

If your package is written in TypeScript then the first approach is favored. Use the --declaration flag to generate declaration files. This way, your declarations and JavaScript always be in sync.

If your package is not written in TypeScript then the second is the preferred approach.

Packages on under the @types organization are published automatically from DefinitelyTyped using the types-publisher tool. To get your declarations published as an @types package, please submit a pull request to https://github.com/DefinitelyTyped/DefinitelyTyped.

So, per the handbook, it looks like the second option is the best choice here though.

joemphilips commented 6 years ago

I don't really agree with the handbook about the place to manage *.d.ts file. since the definition can be more easily diverged from the actual code if we manage in different repo. (See rxjs for an example library which manages definition file in the same repository even it is not written totally in TypeScript.) This also aligns with the trend of monorepo, IMHO.

Ultimately it is up to you, but I still suggest to include definition file in this repository. @ruimarinho

joemphilips commented 6 years ago

oops, sorry. rxjs seems to be written mostly in typescript. Well, I guess it still holds the point.

ruimarinho commented 6 years ago

My concern with adding it to this repository is to make it look like it is supported by the project, which means users will get upset if new releases are not reflected on type definitions.

For now, I think the wisest choice would be keep it under https://github.com/DefinitelyTyped/DefinitelyTyped. I am open to revisit this decision in the future though.

joemphilips commented 6 years ago

:+1:

dancju commented 5 years ago

@joemphilips, It seems you did not post your definition to DefinitelyTyped. I am trying to do so. I am about to push folder [1] to DefinitelyTyped.

DefinitelyTyped requires a test file for all definitions. Some error occurred while I was combining your definition file and test file [2]. Many errors showed up when I tried to compile bitcoin-core-tests.ts with tsc.

Some of those errors seem to be originated from [2], such that should.fail() should take 2 to 4 arguments instead of 0 according to [3]. While most of those errors seem to be caused by wrong definitions. Would you mind fixing it up?

Cheers!

[1] https://github.com/nerdDan/DefinitelyTyped/blob/master/types/bitcoin-core [2] https://github.com/ruimarinho/bitcoin-core/blob/master/test/index_test.js [3] https://shouldjs.github.io/#should-fail

dancju commented 5 years ago

@ruimarinho, would you mind reopen this issue?

joemphilips commented 5 years ago

I will take care, please hang on. @nerdDan

joemphilips commented 5 years ago

While tweaking your fork of DefinitelyTyped, I remembered why I have given up submitting the definition file to DefinitelyTyped. It was because

  1. should.js has awful typing support. It extends primitive type, this is considered as bad practice even in JS alone. Which is even more harsh edge case for TS.
  2. bitcoin-core supports Promise based syntax and callback based syntax. Later is less used especially for newly written code, so I ignored for my typing. But the test requires it.

Sorry @nerdDan , but I give up submitting this code to DefinietelyTyped. I think people desperate for static type can find this PR and tweek the typings for his own use. I won't stop if you want to try to workaround above issues, though.

I think rewriting the whole thing in TypeScript is better way to go in the long term. re-closing for now.

dancju commented 5 years ago

@joemphilips, I totally agree with you on that rewriting the whole thing in TypeScript is better. Thank you for your concern.