-
Notifications
You must be signed in to change notification settings - Fork 39
Conversation
Using the default export only works in ES6+ modules, the export style is technically more correct. Also adds docs and removes extraneous namespace in favour of explicit types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor addition that was missing before that would be useful to add while you're here 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. See questions, ignorable niggles re ordering and one change request.
It's unfortunate that there's documentation copypasta here. This really looks like something that there should be a tool to extract from JSDoc, is that not a thing yet? I reckon it wouldn't be hard.
Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>
Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
Not sure what's up with those tests failing? Seems unrelated to this PR? Anyway, ordering issues fixed. https://github.com/libp2p/js-libp2p-crypto/blob/master/package.json#L30 Note that those 'tests' haven't been 'turned on' in CI yet, and are set to use npx to avoid adding an additional dep to that project before it is actually needed... |
Will have to let @vmx weigh in on whether he things having tests to check this is a good idea or not, it seems good to me but I'm not sure how that actually works and what it would catch. My concern is more about the doubling-up of documentation content. It's a shame we can't just have docs in a single place and that be used for everything (including API docs in the README--my preference) and whatever tooling consumes docs via the types. Anyway, this seems like a good initial step. |
CI just had a hiccup (I just needed to clear the caches). I'm a huge fan of type-checking. I think it leads to better APIs and code. From a technical perspective I prefer Flow, but the TypeScript checker clearly won the game. If there's a non-intrusive way (without code changes) I'm happy to let the checker run. |
Cool, well I might PR the type checking tests here if I get a chance soon-is. Requires no additional code (except maybe some comments), and uses the existing tests which is quite nice. |
Incidentally, is there any chance we could get a patch version release due to these types? I have some downstream work that depends on it (e.g, libp2p/js-peer-id#110), and while I can point to master for now, it might be nice to have the release out as it is technically more correct? cc @vmx. I understand you probably have a release schedule, and so if you'd like to stick to that, I totally understand! |
@carsonfarmer done. |
Using the
default
export is for ES2015 modules, which is fine for typescript etc, but theexport=
style is technically more correct for this module. Also adds docs and removes extraneous namespace in favor of explicit types.