Fix import types

[arjunyel]

https://arethetypeswrong.github.io/?p=bignumber.js%409.1.2

Fix imports and types by copying bignumber.d.ts to bignumber.d.mts.

Test with: npx @arethetypeswrong/cli@latest --pack .

[AdamVig]

I'm trying to migrate from CommonJS to ES Modules and this issue is getting in the way. I'm seeing errors like the following on tsc:

src/index.ts(20,37): error TS2339: Property 'clone' does not exist on type 'typeof import("node_modules/bignumber.js/bignumber")'.
src/index.ts(22,28): error TS2339: Property 'ROUND_HALF_EVEN' does not exist on type 'typeof import("node_modules/bignumber.js/bignumber")'.

Manually applying the changes from this pull request in node_modules/bignumber.js fixes the build. With those changes, npx @arethetypeswrong/cli@latest --pack node_modules/bignumber.js passes.

@MikeMcl Any hesitation about merging this? I'm happy to help in any way I can.

[MikeMcl]

It doesn't feel right to add a new file to the project, bignumber.d.mts, which is the same as bignumber.d.ts except default has become = in the latter.

I think I would prefer to get rid of the type definition files and get AI assistance to add the type definitions as JSDoc comments in the bignumber.js and bignumber.mjs files.

Any thoughts?

[arjunyel]

I don't think there's anything wrong with shipping two type files, they are technically for different environments (CJS and ESM) 😄

[MikeMcl]

It's a bigger npm module.

Anyway, I need more time to look at his. I haven't seen that tool before and it's not clear what configuration etc. is being used.

[arjunyel]

The size addition would just be the type file so wouldn't effect anyone's compiled code, happy to answer any questions that you have

[MikeMcl]

Apparently, the export default BigNumber and export = BigNumber can both be used in the same type definition file, so avoiding the annoying duplication.

[arjunyel]

Apparently, the export default BigNumber and export = BigNumber can both be used in the same type definition file, so avoiding the annoying duplication.

Sorry I don't fully understand what you mean, I went to the master branch and added export = BigNumber; to line 35 of bignumber.d.ts and I get the error: An export assignment cannot be used in a module with other exported elements.

I'm fine with the solution being anything, what I opened a PR with is what I got Are the types wrong to green light

[MikeMcl]

Thanks for testing that.

I think the current type definitions (not this PR) rely on esModuleInterop being set.

[AdamVig]

@MikeMcl

It doesn't feel right to add a new file to the project, bignumber.d.mts, which is the same as bignumber.d.ts except default has become = in the latter.

Good point, I hadn't thought about this. Maybe we could introduce two new entrypoints, bignumber.d.mts and bignumber.d.cts, both of which import and re-export bignumber.d.ts? That would avoid the duplication, though I'm not sure it would work.

I think I would prefer to get rid of the type definition files and get AI assistance to add the type definitions as JSDoc comments in the bignumber.js and bignumber.mjs files.

This approach would still require you to compile and ship .d.ts files, I'm pretty sure. Since bignumber.js and bignumber.mjs each appear to contain full implementations of the library, I think this would result in the same duplication introduced in this pull request (full type definition files for each of bignumber.js and bignumber.mjs).

[MikeMcl]

@AdamVig

Maybe we could introduce two new entrypoints, bignumber.d.mts and bignumber.d.cts, both of which import and re-export bignumber.d.ts?

Yes, good idea, but it may get quite complex as there is also a class, namespace and function to be exported.

I take the point you make about similar duplication.

I'll probably accept this PR soon but I am just reacquainting myself with this stuff first, as I don't like accepting changes I don't fully understand.

[arjunyel]

This is a very important library to the ecosystem so please take all the time you need to feel comfortable, thank you for making this!

I have similar PRs for your other repositories as well:

MikeMcl/decimal.js#234
MikeMcl/decimal.js-light#26

[AdamVig]

@MikeMcl Have you had a chance to consider whether you'd like to merge this change or not?

[MikeMcl]

Fixed in v9.2.0, hopefully.

I used two new entrypoints, as suggested by @AdamVig.

@arjunyel

If you are happy with this, and you alter the others PRs similarly then I will be able to accept them.

Thank you to you both.

[arjunyel]

Fixed in v9.2.0, hopefully.

I used two new entrypoints, as suggested by @AdamVig.

@arjunyel

If you are happy with this, and you alter the others PRs similarly then I will be able to accept them.

Thank you to you both.

@MikeMcl hey I haven't tested it but 9.2.0 might be breaking people

"A require call resolved to an ESM JavaScript file, which is an error in Node and some bundlers. CommonJS consumers will need to use a dynamic import." https://arethetypeswrong.github.io/?p=bignumber.js%409.2.0

[MikeMcl]

@arjunyel

That warning is due to the presence of the "./bignumber.mjs": "./bignumber.mjs", subpath export in the package.json's exports. It exists so users can import directly from the bignumber/bignumber.mjs subpath.

Noone is likely to choose to use require to import from that ES module when they can just as easily use the bignumber/bignumber.js subpath to require the CommonJS version.

I could remove the subpath exports and the warning goes away but it means it would probably break someone's code, and it makes the package less flexible.

[evanshe]

types break when i used this, which is due to the PR
import { BigNumber } from 'bignumber.js'

this works well
import BigNumber from ;bignumber.js

[AdamVig]

Fixed in v9.2.0, hopefully.

I used two new entrypoints, as suggested by @AdamVig.

@arjunyel

If you are happy with this, and you alter the others PRs similarly then I will be able to accept them.

Thank you to you both.

@MikeMcl Works great, thank you so much!

[MikeMcl]

@evanshe

Thanks for the report.

I noticed that too but I have no firm conclusions about it yet.

[MikeMcl]

@evanshe

Hopefully, fixed in v9.2.1, so named import should now work again.