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

Fix import types #371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

arjunyel
Copy link

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
Copy link

AdamVig commented Jan 10, 2025

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
Copy link
Owner

MikeMcl commented Jan 20, 2025

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
Copy link
Author

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

@MikeMcl
Copy link
Owner

MikeMcl commented Jan 20, 2025

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
Copy link
Author

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
Copy link
Owner

MikeMcl commented Jan 21, 2025

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

@arjunyel
Copy link
Author

arjunyel commented Jan 21, 2025

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
Copy link
Owner

MikeMcl commented Jan 21, 2025

Thanks for testing that.

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

@AdamVig
Copy link

AdamVig commented Jan 21, 2025

@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
Copy link
Owner

MikeMcl commented Jan 21, 2025

@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
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

3 participants