fix: move bl and lru-cache types to dependencies

coapjs / node-coap

CoAP - Node.js style

MIT License

531 stars 154 forks source link

fix: move bl and lru-cache types to dependencies #333

Closed JKRhb closed 2 years ago

JKRhb commented 2 years ago

This PR fixes a typescript related issue raised in https://github.com/eclipse/thingweb.node-wot/issues/728 where the types for the bl and lru-cache modules could not be found during transpilation.

relu91 commented 2 years ago

I think something is off, @type/x is meant to be used as devDependecies and everything should work correctly. Because in practice you distribute only the js file and the d.ts types.

JKRhb commented 2 years ago

Hmm, I think at least bl is an exception to the rule as OutgoingMessage extends/implements BufferList from bl. Therefore, the ts transpiler needs the type information to work. As the types are included as a devDependency here, node-wot can't access the types, though. That is the reason the ts transpiler is complaining, I think.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2074267630

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 87.346%

Totals
Change from base Build 1995369947:	0.0%
Covered Lines:	1142
Relevant Lines:	1258

💛 - Coveralls

Apollon77 commented 2 years ago

so we sta with types for bl AND lru-cache in deps?

JKRhb commented 2 years ago

Hmm, yeah, there was also an error thrown for lru-cache. I just noticed that here we have a similar case, as the CoapLRUCache extends LRUCache. So I think both have to go into the regular dependencies.

We could try removing @types/node by the way, not sure if anything directly depends on a definition from the module, though.

JKRhb commented 2 years ago

I now removed @types/node from the regular dependencies. Transpiling in dependent projects should still work (tested out using npm install --production).

JKRhb commented 2 years ago

@Apollon77 Could you release a new version after merging this PR? :)

Apollon77 commented 2 years ago

DO you want to add the version increases to the PR already? ;-)

JKRhb commented 2 years ago

DO you want to add the version increases to the PR already? ;-)

Sure, done :)

danielpeintner commented 2 years ago

Is there any blocking issue left?

Apollon77 commented 2 years ago

@danielpeintner just me having time to merge and release it together with an other fix ...