Adds typescript types + linting/tests #161
Conversation
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
Having the tests here makes sense to me. I was thinking it might be good to have a CI step to run these, but allow the failure for better visibility. The core libp2p team won't have bandwidth to maintain these, but if we can improve visibility into any breaking changes for the bindings we can proactively ping binding contributors to make them aware of the needed changes. @carsonfarmer do you think a CI step would be helpful for visibility? Will the linting catch breaking changes when the js api changes? |
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
|
Your question about testing and CI inspired me to try something out. And it is _much improved! Essentially now, this automatically uses the existing tests to perform the type checking. And right now, it passes 'as is', which is really cool. That means the types match the way they are used now. This means, as code is changed, if the types no longer reflect the usage, it will get flagged if one runs The tests now have some type annotations as comments, but these should be pretty minimal, and doesn't require much in terms of maintenance overhead. They don't affect the running code at all, and its pretty self-explanatory. This is actually really really cool, and I haven't seen this done to this good effect elsewhere. It might provide a really nice template for other libp2p and IPFS projects looking to add types? |
|
So to answer your questions:
Yes!
Yes! |
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
| const id = await crypto.keys.unmarshalPrivateKey(Buffer.from('CAASqAkwggSkAgEAAoIBAQCk0O+6oNRxhcdZe2GxEDrFBkDV4TZFZnp2ly/dL1cGMBql/8oXPZgei6h7+P5zzfDq2YCfwbjbf0IVY1AshRl6B5VGE1WS+9p1y1OZxJf5os6V1ENnTi6FTcyuBl4BN8dmIKOif0hqgqflaT5OhfYZDXfbJyVQj4vb2+Stu2Xpph3nwqAnTw/7GC/7jrt2Cq6Tu1PoZi36wSwEPYW3eQ1HAYxZjTYYDXl2iyHygnTcbkGRwAQ7vjk+mW7u60zyoolCm9f6Y7c/orJ33DDUocbaGJLlHcfd8bioBwaZy/2m7q43X8pQs0Q1/iwUt0HHZj1YARmHKbh0zR31ciFiV37dAgMBAAECggEADtJBNKnA4QKURj47r0YT2uLwkqtBi6UnDyISalQXAdXyl4n0nPlrhBewC5H9I+HZr+zmTbeIjaiYgz7el1pSy7AB4v7bG7AtWZlyx6mvtwHGjR+8/f3AXjl8Vgv5iSeAdXUq8fJ7SyS7v3wi38HZOzCEXj9bci6ud5ODMYJgLE4gZD0+i1+/V9cpuYfGpS/gLTLEMQLiw/9o8NSZ7sAnxg0UlYhotqaQY23hvXPBOe+0oa95zl2n6XTxCafa3dQl/B6CD1tUq9dhbQew4bxqMq/mhRO9pREEqZ083Uh+u4PTc1BeHgIQaS864pHPb+AY1F7KDvPtHhdojnghp8d70QKBgQDeRYFxo6sd04ohY86Z/i9icVYIyCvfXAKnaMKeGUjK7ou6sDJwFX8W97+CzXpZ/vffsk/l5GGhC50KqrITxHAy/h5IjyDODfps7NMIp0Dm9sO4PWibbw3OOVBRc8w3b3i7I8MrUUA1nLHE1T1HA1rKOTz5jYhE0fi9XKiT1ciKOQKBgQC903w+n9y7M7eaMW7Z5/13kZ7PS3HlM681eaPrk8J4J+c6miFF40/8HOsmarS38v0fgTeKkriPz5A7aLzRHhSiOnp350JNM6c3sLwPEs2qx/CRuWWx1rMERatfDdUH6mvlK6QHu0QgSfQR27EO6a6XvVSJXbvFmimjmtIaz/IpxQKBgQDWJ9HYVAGC81abZTaiWK3/A4QJYhQjWNuVwPICsgnYvI4Uib+PDqcs0ffLZ38DRw48kek5bxpBuJbOuDhro1EXUJCNCJpq7jzixituovd9kTRyR3iKii2bDM2+LPwOTXDdnk9lZRugjCEbrPkleq33Ob7uEtfAty4aBTTHe6uEwQKBgQCB+2q8RyMSXNuADhFlzOFXGrOwJm0bEUUMTPrduRQUyt4e1qOqA3klnXe3mqGcxBpnlEe/76/JacvNom6Ikxx16a0qpYRU8OWz0KU1fR6vrrEgV98241k5t6sdL4+MGA1Bo5xyXtzLb1hdUh3vpDwVU2OrnC+To3iXus/b5EBiMQKBgEI1OaBcFiyjgLGEyFKoZbtzH1mdatTExfrAQqCjOVjQByoMpGhHTXwEaosvyYu63Pa8AJPT7juSGaiKYEJFcXO9BiNyVfmQiqSHJcYeuh+fmO9IlHRHgy5xaIIC00AHS2vC/gXwmXAdPis6BZqDJeiCuOLWJ94QXn8JBT8IgGAI', 'base64')) | ||
|
|
||
| const msg = Buffer.from('hello') | ||
|
|
||
| // browser | ||
| const dec1 = await id.decrypt(Buffer.from('YRFUDx8UjbWSfDS84cDA4WowaaOmd1qFNAv5QutodCKYb9uPtU/tDiAvJzOGu5DCJRo2J0l/35P2weiB4/C2Cb1aZgXKMx/QQC+2jSJiymhqcZaYerjTvkCFwkjCaqthoVo/YXxsaFZ1q7bdTZUDH1TaJR7hWfSyzyPcA8c0w43MIsw16pY8ZaPSclvnCwhoTg1JGjMk6te3we7+wR8QU7VrPhs54mZWxrpu3NQ8xZ6xQqIedsEiNhBUccrCSzYghgsP0Ae/8iKyGyl3U6IegsJNn8jcocvzOJrmU03rgIFPjvuBdaqB38xDSTjbA123KadB28jNoSZh18q/yH3ZIg==', 'base64')) |
There was a problem hiding this comment.
Note that the type-checking helped to find this erroneous await :)
| @@ -12,7 +12,8 @@ const crypto = require('../../src') | |||
|
|
|||
| describe('without libp2p-crypto-secp256k1 module present', () => { | |||
| before(() => { | |||
| sinon.replace(crypto.keys.supportedKeys, 'secp256k1', null) | |||
| const empty = null | |||
There was a problem hiding this comment.
Needed for type checking... doesn't affect tests
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
|
If you needed another reason to support types: And feast your eyes on lovely docs! |
There was a problem hiding this comment.
@carsonfarmer this is looking good, sorry for the delay I've been ill.
The docs are really nice, it's definitely a benefit I'd like to look at integrating in the future.
This could be turned on in CI (I haven't yet, didn't want to presume).
I think it would be nice to include this as a new stage (that's allowed to fail for now). That way we don't have to rely on running this locally to see issues. I think we could do this as a separate PR though in order to get the types merged in sooner. Do you have a preference?
|
Yeh I'd love to get the types in ASAP as I'll use them right away, and they provide the basis for other types such as |
|
Sounds good, released in 0.17.2 |
|
Woohoo! Cheers 👍 |
This preliminary PR adds Typescript types for libp2p-crypto. It includes linting-based 'tests', which were derived from existing libp2p-crypto tests. Is also includes one new dependency,
dtslint, which is the test/lint framework used by https://github.com/DefinitelyTyped/DefinitelyTyped. The setup provided here is what is required by DefinitelyTyped. With that in mind, I would be more than happy to remove tests and additional deps and stick to only including the types themselves if this is preferable? Note also that the types include significant documentation for almost all methods and classes. This documentation could be used to derive external documentation if that is desired.Alternatively, if @jacobheun or any other maintainers would prefer to have these types with DefinitelyTyped, I'm happy to submit there instead!