hildjj / dohdec

Lookup and decode DNS records using DNS-over-HTTPS (DoH)
MIT License
19 stars 5 forks source link

Missing declarations in upgrade from v3 to v5 #33

Closed gnarea closed 2 years ago

gnarea commented 2 years ago

Dependabot created a PR to upgrade from v3 to v5.0.1, but the tests are breaking due to some missing types:

node_modules/dohdec/types/dnsUtils.d.ts:193:25 - error TS7016: Could not find a declaration file for module 'dns-packet'. '/home/runner/work/relaynet-core-js/relaynet-core-js/node_modules/dns-packet/index.js' implicitly has an 'any' type.
  Try `npm install @types/dns-packet` if it exists or add a new declaration (.d.ts) file containing `declare module 'dns-packet';`

193 import * as packet from "dns-packet";
                            ~~~~~~~~~~~~

node_modules/dohdec/types/doh.d.ts:146:25 - error TS7016: Could not find a declaration file for module 'dns-packet'. '/home/runner/work/relaynet-core-js/relaynet-core-js/node_modules/dns-packet/index.js' implicitly has an 'any' type.
  Try `npm install @types/dns-packet` if it exists or add a new declaration (.d.ts) file containing `declare module 'dns-packet';`

146 import * as packet from "dns-packet";
                            ~~~~~~~~~~~~

node_modules/dohdec/types/dot.d.ts:48:43 - error TS2694: Namespace '"crypto"' has no exported member 'X509Certificate'.

48     static hashCert(cert: Buffer | crypto.X509Certificate, hashAlg?: string): string;
                                             ~~~~~~~~~~~~~~~

node_modules/dohdec/types/dot.d.ts:188:25 - error TS7016: Could not find a declaration file for module 'dns-packet'. '/home/runner/work/relaynet-core-js/relaynet-core-js/node_modules/dns-packet/index.js' implicitly has an 'any' type.
  Try `npm install @types/dns-packet` if it exists or add a new declaration (.d.ts) file containing `declare module 'dns-packet';`

188 import * as packet from "dns-packet";
                            ~~~~~~~~~~~~

Found 4 errors.
hildjj commented 2 years ago

Do you think I should take a runtime dependency on @types/dns-packet instead of asking Typescript users to do so? I agree that at the very least, I need to update the docs.

hildjj commented 2 years ago

The worst part is that @types/dns-packet is pretty out of date. Maybe I should just use dns-packet without type info.

gnarea commented 2 years ago

Hey Joe. Thanks for looking into this issue. As a user, I'd rather not have a @.**/` package as a runtime dependency -- especially if I have to install it manually, and also because I can't add a comment to package.json to explain why we need it.

On Mon, 13 Dec 2021, 19:24 Joe Hildebrand, @.***> wrote:

Do you think I should take a runtime dependency on @types/dns-packet instead of asking Typescript users to do so? I agree that at the very least, I need to update the docs.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hildjj/dohdec/issues/33#issuecomment-992796037, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2JR4KHIOQAETROBXICWLUQZB7HANCNFSM5J6HN6ZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

hildjj commented 2 years ago

I'll see if I can work around needing it at all. But in a typescript project, I would be surprised if you didn't have any other @types/ dev-dependencies.

gnarea commented 2 years ago

Thanks. I'd imagine the issue is that dohdec typyings are leaking the typings of dns-package, so I wonder if there's a way to hide them from the public interface.

I do have some @.**/in devPackages but I was thinking of runtime dependencies`... Which, as I'm writing this, I'm just realising that Jest must be running the original TS files instead of the transpiled JS. So yes, adding one more typings package wouldn't be the end of the world.

On Mon, 13 Dec 2021, 19:34 Joe Hildebrand, @.***> wrote:

I'll see if I can work around needing it at all. But in a typescript project, I would be surprised if you didn't have any other @types/ dev-dependencies.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hildjj/dohdec/issues/33#issuecomment-992803508, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2JRZHIDVN3MEO2AC5TRDUQZDEHANCNFSM5J6HN6ZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

hildjj commented 2 years ago

Fixed in v5.0.3 which I just published.

gnarea commented 2 years ago

Amazing. Thank you so much!

On Mon, 13 Dec 2021, 21:32 Joe Hildebrand, @.***> wrote:

Fixed in v5.0.3 which I just published.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hildjj/dohdec/issues/33#issuecomment-992930011, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2JRYGPWOCULRTQLANGBDUQZRADANCNFSM5J6HN6ZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.