search

coapjs / node-coap

CoAP - Node.js style
MIT License
533 stars 156 forks source link

refactor: replace declaration files with JSDoc comments #289

Closed JKRhb closed 3 years ago

JKRhb commented 3 years ago

This PR is supposed to be a more elegant alternative to #285.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1298164523

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/observe_read_stream.js 1 2 50.0%
lib/observe_write_stream.js 3 4 75.0%
lib/parameters.js 7 8 87.5%
lib/segmentation.js 5 6 83.33%
index.ts 43 45 95.56%
lib/retry_send.js 1 5 20.0%
lib/helpers.js 7 12 58.33%
lib/server.js 38 45 84.44%
lib/agent.js 15 23 65.22%
<!-- Total: 151 181 83.43% -->
Files with Coverage Reduction New Missed Lines %
lib/parameters.js 2 69.23%
lib/cache.js 3 86.49%
lib/retry_send.js 22 29.09%
lib/helpers.js 40 56.8%
lib/option_converter.js 43 43.8%
<!-- Total: 110 -->
Totals Coverage Status
Change from base Build 1287269036: -11.05%
Covered Lines: 974
Relevant Lines: 1192
💛 - Coveralls
coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1292481937

Totals Coverage Status
Change from base Build 1287269036: 0.04%
Covered Lines: 1072
Relevant Lines: 1163
💛 - Coveralls
relu91 commented 3 years ago

I think the JSDoc approach is more maintainable since it put the type definition closer to the code. Sadly, I did not have the experience with porting a JS project to TS. I would say that this is the right direction though.

JKRhb commented 3 years ago

I think the JSDoc approach is more maintainable since it put the type definition closer to the code. Sadly, I did not have the experience with porting a JS project to TS. I would say that this is the right direction though.

Awesome, thanks for the feedback! :) I think I actually found a setup that offers the best of both worlds, keeping the project structure mostly as it is while providing strict(er) type checking based on the JSDoc comments. I need to add a couple of commits then this PR will be able to be merged :)

JKRhb commented 3 years ago

With the exception of some cleanup this PR should now be finally finished :) It should provide typing support for the whole project now. Unfortunately, this isn't possible without having some kind of compilation step as JSDoc comments are only parsed if depending Typescript have a value > 0 for maxNodeModuleJsDepth which should not be the case too often. I therefore added a .ts file back and let tsc compile the whole project before testing and publishing.

In the process of working on this PR I noticed that there is an incorrect type definition for the bl module. I opened https://github.com/DefinitelyTyped/DefinitelyTyped/pull/56130 to this fix issue. Once it is merged, this PR will be ready to be merged as well.

JKRhb commented 3 years ago

Closed in favor of #293.