search

multiformats / js-cid

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

fix: more correct type defs + docs #102

Closed carsonfarmer closed 4 years ago

carsonfarmer commented 4 years ago

Using the default export is for ES2015 modules, which is fine for typescript etc, but the export= style is technically more correct for this module. Also adds docs and removes extraneous namespace in favor of explicit types.

carsonfarmer commented 4 years ago

Not sure what's up with those tests failing? Seems unrelated to this PR? Anyway, ordering issues fixed. Regarding JSDoc, there are tools to do some of this extraction tho pretty error-prone, and there are also tools to do the reverse (generate docs from the type defs). One thing I did with libp2p-crypto is actually added the ability to do type-checking as a 'test/lint' step. This is a nice way to catch and flag API and documentation changes... the test integration is pretty minimal if we wanted to explore that here as well?

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...

rvagg commented 4 years ago

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.

vmx commented 4 years ago

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.

carsonfarmer commented 4 years ago

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.

carsonfarmer commented 4 years ago

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, https://github.com/libp2p/js-peer-id/pull/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!

vmx commented 4 years ago

@carsonfarmer done.