multiformats / js-cid

CID implementation in JavaScript
MIT License
97 stars 39 forks source link

Add typescript types #94

Closed carsonfarmer closed 4 years ago

carsonfarmer commented 4 years ago

Offering to submit and maintain these types. Given the relative stability of js-cids, this seems like a low maintenance PR.

carsonfarmer commented 4 years ago

Woo hoo, thanks @rvagg. What's the merge policy here? Do we need more than one approval? Should I 'ping' someone to take a look?

vmx commented 4 years ago

Somehow the CI is failing on Windows, but it's certainly not caused by this PR. Hence merging.

rvagg commented 4 years ago

Ping also @momack2 who had opinions on TS bindings and the associated maintenance burden.

To restate what I've said elsewhere, my thoughts on this in general:

carsonfarmer commented 4 years ago

đź’Ż agree @rvagg! In fact, the types can also be used to great effect for documentation. The doc strings can be added to the types directly, and TypeDoc or event JSDoc can be used to build docs from them, thereby increasing their usefulness and likelihood of being maintained.

mikeal commented 4 years ago

I am normally quite resistant to adding typedefs to libraries because of the burden imposed when altering the library.

However, this library should be considered “done” at this point. Any modification to the API and its types should be considered a breaking change and having a typedef test that validates we don’t do this would be a positive addition to the project. We rely on this API all over the place and can’t really tolerate it changing behavior without a lot of notice.

In fact, @vmx and I have talked about a change to this library that would make it possible to get rid of the multi-codec table as a default dependency. Having a typedef test would help us surface all the potential bugs in the exposed properties when we do that.

carsonfarmer commented 4 years ago

These points are all well taken. In fact, the stability of this library was why I 'targeted' it for a types-based PR :) The DefinitelyTyped repo has some really robust typedef testing tooling (for obvious reasons). It might be a bit burdensome for now to incorporate any of that typedef testing here, but perhaps it could be incorporated into any existing testing infrastructure that this and other ecosystem libs use? This probably isn't the place for such a discussion, but something to keep in mind.