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

decimal128 does not play nicely with tsc #119

Closed
ljani opened this issue Oct 15, 2024 · 4 comments · Fixed by #122 or #123
Closed

decimal128 does not play nicely with tsc #119

ljani opened this issue Oct 15, 2024 · 4 comments · Fixed by #122 or #123

Comments

@ljani
Copy link

ljani commented Oct 15, 2024

Summary

I tried to use this with Vite's react-ts template. While it works nicely in npm run dev mode, npm run build fails, because TypeScript tries to compile package's .mts file for unknown reason.

Steps to reproduce

Here's a repository where you can reproduce the issue:

git clone https://github.com/ljani/decimal128-tsc-issue
cd decimal128-tsc-issue
npm run ci
npm run build

Expected result

The build completes fine.

Actual result

The build fails with:

node_modules/decimal128/src/Decimal.mts:3:7 - error TS6133: 'ratOne' is declared but its value is never read.

3 const ratOne = new Rational(1n, 1n);
        ~~~~~~

node_modules/decimal128/src/Decimal.mts:4:7 - error TS6133: 'ratTen' is declared but its value is never read.

4 const ratTen = new Rational(10n, 1n);
        ~~~~~~

node_modules/decimal128/src/Decimal.mts:49:14 - error TS6133: 'dec' is declared but its value is never read.

49         let [dec, exp] = s.split(/[eE]/);
                ~~~

node_modules/decimal128/src/Decimal128.mts:45:5 - error TS6133: 'd' is declared but its value is never read.

45     d: "0" | "-0" | Rational,
       ~

node_modules/decimal128/src/Rational.mts:7:5 - error TS6133: 'ROUNDING_MODE_HALF_EXPAND' is declared but its value is never read.

7     ROUNDING_MODE_HALF_EXPAND,
      ~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:321:9 - error TS6133: 'penultimateDigit' is declared but its value is never read.

321         penultimateDigit: Digit,
            ~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:337:9 - error TS6133: 'penultimateDigit' is declared but its value is never read.

337         penultimateDigit: Digit,
            ~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:354:9 - error TS6133: 'penultimateDigit' is declared but its value is never read.

354         penultimateDigit: Digit,
            ~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:355:9 - error TS6133: 'finalDigit' is declared but its value is never read.

355         finalDigit: Digit,
            ~~~~~~~~~~


Found 9 errors.

Steps to reproduce (the long way)

npm create vite@latest decimal128-tsc-issue -- --template react-ts
cd decimal128-tsc-issue
npm install
npm install --save decimal128
nvim ./src/App.tsx
# Use decimal128 in App.tsx
npm run build

→ same result

Notes

  • As this is probably related to packaging of the project, chore: build project to emit cjs and esm using rollup  #93 might fix this, but I'm not sure?
  • This is the only package I've encountered this issue, so I don't think Vite's template is misconfigured
  • It is tsc -b, which fails, not vite build
  • Thanks for your work, decimal128 seems a welcome addition to the ecosystem
@ljani ljani changed the title Package does not play nicely with tsc decimal128 does not play nicely with tsc Oct 15, 2024
@jessealama
Copy link
Owner

It looks like there's some dead code in the examples you give. I'm not sure why those are being regarded as errors, but I can remove the dead code and we can try again.

I've also merged in some changes, in #121 , that might be relevant.

@jessealama jessealama linked a pull request Nov 15, 2024 that will close this issue
@ljani
Copy link
Author

ljani commented Nov 15, 2024

It looks like there's some dead code in the examples you give. I'm not sure why those are being regarded as errors, but I can remove the dead code and we can try again.

Thanks for looking into this. My point was that it shouldn't even touch eg. Decimal.mts, but use Decimal.mjs and Decimal.d.mts from dist/esm/, right?

I've also merged in some changes, in #121 , that might be relevant.

Judging by the title it sounds a good change. However, I updated decimal128 to 20.1.0 in ljani/decimal128-tsc-issue@1bc2393 and the issue still persists, though the errors are a bit different:

node_modules/decimal128/src/Rational.mts:7:5 - error TS6133: 'ROUNDING_MODE_HALF_EXPAND' is declared but its value is never read.

7     ROUNDING_MODE_HALF_EXPAND,
      ~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:321:9 - error TS6133: 'penultimateDigit' is declared but its value is never read.

321         penultimateDigit: Digit,
            ~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:337:9 - error TS6133: 'penultimateDigit' is declared but its value is never read.

337         penultimateDigit: Digit,
            ~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:354:9 - error TS6133: 'penultimateDigit' is declared but its value is never read.

354         penultimateDigit: Digit,
            ~~~~~~~~~~~~~~~~

node_modules/decimal128/src/Rational.mts:355:9 - error TS6133: 'finalDigit' is declared but its value is never read.

355         finalDigit: Digit,
            ~~~~~~~~~~


Found 5 errors.

@ljani
Copy link
Author

ljani commented Nov 15, 2024

I took a small peek in package.json:

decimal128/package.json

Lines 57 to 59 in b218eaa

"exports": {
".": "./src/Decimal128.mjs"
}
should probably be something along the lines:

{
    "exports": {
        "types": "./dist/esm/Decimal128.d.mts",
        "default": "./dist/esm/Decimal128.mjs"
    }
}

I took this inspiration from ky's package.json. However, it is esm-only, so you might get a better example from somewhere else.

Changing this in node_modules/ makes my example to compile without any issues.

I'm not fully sure how to export CJS though. Maybe:

{
    "exports": {
        "types": "./dist/esm/Decimal128.d.mts",
        "require": "./dist/cjs/Decimal128.cjs"
        "default": "./dist/esm/Decimal128.mjs"
    }
}

EDIT: axios@1.7.7 seems to use the following, so my CJS example is wrong:

{
  "exports": {
    ".": {
      "types": {
        "require": "./index.d.cts",
        "default": "./index.d.ts"
      },
      "browser": {
        "require": "./dist/browser/axios.cjs",
        "default": "./index.js"
      },
      "default": {
        "require": "./dist/node/axios.cjs",
        "default": "./index.js"
      }
    }
}

So, maybe this would work:

{
    "exports": {
        "types": {
            "require": "./dist/cjs/Decimal128.d.mts"
            "default": "./dist/esm/Decimal128.d.mts"
        },
        "default": {
            "require": "./dist/cjs/Decimal128.cjs"
            "default": "./dist/esm/Decimal128.mjs"
        }
    }
}

or just this?

{
    "exports": {
        "types": {
            "require": "./dist/cjs/Decimal128.d.mts"
            "default": "./dist/esm/Decimal128.d.mts"
        },
        "require": "./dist/cjs/Decimal128.cjs"
        "default": "./dist/esm/Decimal128.mjs"
    }
}

@amoshydra
Copy link
Contributor

I am able to reproduce the issue too using 20.1.0 using npm run build command in vite.

Changing the exports to refer to dist instead of src should work.

image

I have raised another PR here -> #123

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 a pull request may close this issue.

3 participants