Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NPM module does not include type declaration files #6

Closed
GoudaSoftware opened this issue Dec 14, 2024 · 5 comments
Closed

NPM module does not include type declaration files #6

GoudaSoftware opened this issue Dec 14, 2024 · 5 comments

Comments

@GoudaSoftware
Copy link
Contributor

I'll start off by saying I'm rather inexperienced javascript and typescript, so forgive me if this sounds imprecise or my understanding is simply incorrect.

I'm writing a node.js project using typescript. I imported banani using import statements, as I'm preferring these over require. e.g.
import {Wallet, RPC} from "banani";

When running tsc in my project I get a number of errors such as:

node_modules/banani/util.ts:125:7 - error TS6133: 'NANO_DECIMALS' is declared but its value is never read.

125 const NANO_DECIMALS: number = 30;

or

node_modules/banani/wallet.ts:89:32 - error TS2722: Cannot invoke an object which is possibly 'undefined'.

89     if (gen_work) work = await this.work_function(s_block_hash);

It took me a while to figure what was going. It was because I had typescript set to strict. Here is my tsconfig, which was mostly generated by the firebase cli:

{
  "compilerOptions": {
    "module": "commonjs",
    "noImplicitReturns": true,
    "noUnusedLocals": true,
    "outDir": "lib",
    "sourceMap": true,
    "strict": true,
    "target": "es2022",
    "esModuleInterop": true,
  },
  "compileOnSave": true,
  "include": [
    "src"
  ],
  "exclude": [
    "node_modules"
  ]
}

In order to successfully compile after importing banani, I need to set strict and noUnusedLocal to false.


Ideally I wouldn't have to disable strict mode because of a dependency. After a bit of reading, my understanding is that the banani module should be supplying .d.ts declaration files instead of the .ts files it has now.
banani_node_modules

Does that sound right?

@stjet
Copy link
Owner

stjet commented Dec 14, 2024

I don't think that sounds right. My understanding is that .d.ts files are to generate type declaration for Javascript-only code. Say, you have this very old library written in Javascript (not Typescript!). But you still want type information. So instead of rewriting the whole library in Typescript you can just write .d.ts files for the library.

Typescript code already comes with the type information, and so does not need .d.ts files. What you have are compiler errors anyways, so completely unrelated.

Anyways, on to the actual issue, that the library errors when compiling with strict and noUnusedLocal set to true. Yeah, that seems annoying, I wouldn't want to be forced to change those settings for the entire project for just one dependency. It would be nice if Typescript allowed this kind of control on a per-package basis but I guess that isn't the world we live in.

error TS6133: 'NANO_DECIMALS' is declared but its value is never read.

I do not want to remove this line, in case library users want to use it. I think changing const to export const will fix this. If not, I will slap on a ts-ignore comment.

error TS2722: Cannot invoke an object which is possibly 'undefined'.

Ok, that is a legitimate issue. Will fix. Probably if (gen_work) to if (gen_work && this.work_function) should fix it.

Thanks for reporting.

@stjet
Copy link
Owner

stjet commented Dec 14, 2024

Oh, I did discover that the js files tweetnacl_mod.js and blake2b were missing .d.ts files. I'm adding the .d.ts files for blake2b, but for tweetnacl_mod.js .d.ts files are not going to be easy to make and going to bring zero benefit to anyone, so I just slapped a // @ts-ignore before it. Unfortunate, but harmless.

@stjet
Copy link
Owner

stjet commented Dec 14, 2024

@GoudaSoftware In banani v1.0.5 (commit 295e180), should now compile with strict: true and noUnusedLocals: true, and nothing should be broken. Let me know if it works, so I can close the issue. More importantly, let me know if it doesn't work. Thanks again.

@GoudaSoftware
Copy link
Contributor Author

Wow! Thanks for such a quick response. Yeah the issue definitely is that typescript enforces rules for dependency sources when compiling. Which seems like a reasonable thing to skip. And after reading a bit more, as you say, a lot of resources do seem to indicate .d.ts files are more valuable for javascript-only code. But the "development lead for the typescript team" is saying "The only correct path forward is to not have a .ts file in your node_modules". 😕 And given my inexperience with the language, I'm not truly understanding what the typescript documentation is recommending.

Honestly, I feel like there's still some improvement that could be made here, but I couldn't tell you exactly what it is. And I'm ok with that. I just tried 1.0.5 and I think it fixes my issues at least 😄. Feel free to close this issue if you would like.

As an aside, thanks for this library. I'm just learning to work with banano and this is way easier to understand than bananojs.

@stjet
Copy link
Owner

stjet commented Dec 14, 2024

way easier to understand than bananojs

That was the goal! Glad to hear that, and glad that the issue seems to be resolved.

@stjet stjet closed this as completed Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants