Skip to content
This repository has been archived by the owner on Sep 3, 2021. It is now read-only.

Add typescript types #94

Merged
merged 3 commits into from
Dec 2, 2019
Merged

Add typescript types #94

merged 3 commits into from
Dec 2, 2019

Conversation

carsonfarmer
Copy link
Contributor

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

Offering to submit and maintain these types. Given the relative stability of js-cids, this seems like a low maintenance PR.
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
src/index.d.ts Outdated Show resolved Hide resolved
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
@carsonfarmer
Copy link
Contributor Author

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
Copy link
Member

vmx commented Dec 2, 2019

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

@vmx vmx merged commit 4cf17bb into multiformats:master Dec 2, 2019
@rvagg
Copy link
Member

rvagg commented Dec 2, 2019

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:

  • TS bindings are really helpful, especially to people who consume TS directly but also to many of the rest of us via tooling, like VS Code which makes great use of them where present.
  • TS is great for large, complex projects and is only growing in popularity, so we (JS folks) should put in some effort to at least get familiarity, if not start adopting it in strategic ways.
  • In my experience, people who care about good TS bindings and who consume them, will show up and fix anything amiss in projects that have them. A few of my popular projects have TS bindings that I don't fully understand but those files have their own communities around them, people show up with enhancements and fixes, and there are even regulars that I can call on to come help out if there was a hint that something might break with a change. https://github.com/rvagg/node-worker-farm/blob/master/index.d.ts is probably my favourite example of this.

@carsonfarmer carsonfarmer deleted the patch-1 branch December 2, 2019 23:13
@carsonfarmer
Copy link
Contributor Author

💯 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
Copy link
Contributor

mikeal commented Dec 3, 2019

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
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants