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

Type definitions are broken in 4.0.0-beta.1 #169

Closed
4 tasks done
remcohaszing opened this issue Jul 29, 2023 · 10 comments
Closed
4 tasks done

Type definitions are broken in 4.0.0-beta.1 #169

remcohaszing opened this issue Jul 29, 2023 · 10 comments
Labels
bug This points to a verified bug in the code needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue

Comments

@remcohaszing
Copy link
Contributor

Checklist

  • I have looked into the Readme and have not found a suitable solution or answer.
  • I have searched the issues and have not found a suitable solution or answer.
  • I have searched the Auth0 Community forums and have not found a suitable solution or answer.
  • I agree to the terms within the Auth0 Code of Conduct.

Description

Type definitions are broken for both CJS and ESM.

Reproduction

Create the following files:

// package.json
{
  "dependencies": {
    "jwt-decode": "4.0.0-beta.1",
    "typescript": "5.1.6"
  }
}
// tsconfig.json
{
  "compilerOptions": {
    "checkJs": true,
    "moduleResolution": "node16",
    "strict": true
  }
}
// index.cjs
const jwtDecode = require('jwt-decode');

jwtDecode.default('')

This yields the following type error:

$ tsc
node_modules/jwt-decode/build/typings/index.d.ts:1:57 - error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 import { JwtDecodeOptions, JwtHeader, JwtPayload } from "./global";
                                                          ~~~~~~~~~~

node_modules/jwt-decode/build/typings/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './global';
                ~~~~~~~~~~

script.cjs:1:27 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("jwt-decode")' call instead.

1 const jwtDecode = require('jwt-decode');
                            ~~~~~~~~~~~~


Found 3 errors in 2 files.

Errors  Files
     2  node_modules/jwt-decode/build/typings/index.d.ts:1
     1  script.cjs:1

This shows 2 errors:

  1. According to the type definitions jwt-decode is ESM only, although there is a CJS entrypoint. To solve this, the exports in package.json should look something like this:
    {
        "exports": {
             ".": {
                 "require": {
                     "types": "./build/typings/index.d.cts",
                     "default": "./build/cjs/jwt-decode.js"
                 },
                 "import": {
                     "types": "./build/typings/index.d.ts",
                     "default": "./build/esm/jwt-decode.js"
                 }
             }
         }
     }
  2. All imports should have the appropriate extension (.js in this case)
  3. Even if the above 2 are fixed, the type definitions of the CJS module should look like this:
    declare function jwtDecode(/* … */) // …
    
     declare namespace jwtDecode {
      class InvalidTokenError extends Error {
        // …
      }
    }
    
    export = jwtDecode

Additional context

The current build setup is relatively complex. Dual publishing is pretty complex. I’m not sure how to fix it in this setup. An alternative solution is to use a named export instead of a default export.

jwt-decode version

4.0.0-beta.1

Which browsers have you tested in?

Other

@remcohaszing remcohaszing added the bug This points to a verified bug in the code label Jul 29, 2023
@frederikprijck
Copy link
Member

frederikprijck commented Jul 29, 2023

Thanks for letting us know.

Interestingly, types work fine for me with ESM, and CJS doesn't give any issue with both module resolution set to node and node16 (but I did notice types are any, which we need to solve 🤔 ).

This is what I use to test: https://github.com/frederikprijck/jwt-decode-test

You can run the following commands:

  • npm run test:esm
  • npm run test:cjs
  • npm run test:cjs:node

When running the cjs scripts, make sure to set module to commonjs in package.json.

All of these commands work fine for me, so I must be missing something to reproduce this. Could you perhaps let me know how you can reproduce it in that example?

image image

@frederikprijck
Copy link
Member

frederikprijck commented Jul 29, 2023

Ignore the above, I can perfectly reproduce the same issue by just running tsc on the sample repo. Which is odd, given all other commands compile and run just fine.

Adding the file extension to the import paths solves the issue, updating the exports field in package.json doesn't seem to have any additional affect based on what I can see, will need to look into this further.

@frederikprijck frederikprijck added the needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue label Jul 29, 2023
@frederikprijck
Copy link
Member

frederikprijck commented Jul 29, 2023

I opened a PR, that does seem to fix the issue with tsc, but will need a follow up for the commonjs types (but it should at least run).

@remcohaszing any chance you can try that branch and see if it solves it on your end as well? If not, could you elaborate how to update my sample linked above to reproduce what you are seeing?

@jonkoops
Copy link
Contributor

I have some things in the works to fix up the types, both for CommonJS and ESM. We will have to follow the best practices as outlined by the TypeScript handbook (see my earlier comment #151 (comment)):

It’s important to note that the CommonJS entrypoint and the ES module entrypoint each needs its own declaration file, even if the contents are the same between them. Every declaration file is interpreted either as a CommonJS module or as an ES module, based on its file extension and the "type" field of the package.json, and this detected module kind must match the module kind that Node will detect for the corresponding JavaScript file for type checking to be correct. Attempting to use a single .d.ts file to type both an ES module entrypoint and a CommonJS entrypoint will cause TypeScript to think only one of those entrypoints exists, causing compiler errors for users of the package.

@remcohaszing
Copy link
Contributor Author

I fixed the test cases in frederikprijck/jwt-decode-test#1. Run tsc from that PR to see all the errors.

I recommend to verify the package against Are The Types Wrong to make sure it’s built correctly.

@jonkoops
Copy link
Contributor

Got a PR up under #174 to fix this issue.

@cristobal
Copy link
Contributor

I tested beta 1.0 with the current tsconfig which is the one we use at work

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "display": "TS Config",
  "compilerOptions": {
    "allowJs": true,
    "checkJs": true,
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "isolatedModules": true,
    "lib": [
      "ESNext",
      "DOM",
      "DOM.Iterable"
    ],
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "noImplicitOverride": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "resolveJsonModule": true,
    "strict": true,
    "target": "ESNext",
    "useUnknownInCatchVariables": false,
    "verbatimModuleSyntax": true
  },
  ...
}

Seems to work fine, but the same as pointed by others that the package is commonjs masquerading as ESM.

@frederikprijck
Copy link
Member

If I understand correctly, the issues in this thread should be resolved with the current beta branch? Is that correct @remcohaszing?

@remcohaszing
Copy link
Contributor Author

Yep, it’s working correctly now 🎉

@frederikprijck
Copy link
Member

Released as v4.0.0-beta.2!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This points to a verified bug in the code needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue
Projects
None yet
Development

No branches or pull requests

4 participants