Ensure types are bundled and correctly linked #174

jonkoops · 2023-07-31T15:24:59Z

Ensures that emitted types are bundled into a single file, and that both ESM and CommonJS have their own type definition files linked. This is done by adding a new script to generate the types using tsup, which can bundle the types, unlike the TypeScript (see microsoft/TypeScript#4433).

Closes #169

jonkoops · 2023-07-31T15:33:58Z

@remcohaszing I took your suggestion from #169 (comment), and used the Are the types wrong? CLI to verify the package:

I'll see if I can introduce this as part of the build process in a subsequent PR, for now I wanted to keep this one lean and to the point. In the meantime, can I ask you to take a look at the changes here and let me know if this also matches your expectations?

jonkoops · 2023-07-31T15:35:01Z

package.json

-  "type": "module",
-  "exports": {
-    ".": {
-      "require": "./build/cjs/jwt-decode.js",
-      "import": "./build/esm/jwt-decode.js",
-      "types": "./build/typings/index.d.ts"
-    }


I've moved this up next to the main and module definitions, so that all the paths that export stuff are in the same place.

remcohaszing · 2023-07-31T15:56:17Z

This helps, but the CJS types are still wrong (ATTW is nice, but not yet perfect). They should not define export default jwtDecode, but:

declare function jwtDecode(/* … */) // …


declare namespace jwtDecode {
  class InvalidTokenError extends Error {
    // …
  }
}

export = jwtDecode

The simplest solution is to simply not use default exports, but named exports.

Also I believe tsup can just replace rollup here.

jonkoops · 2023-07-31T16:31:44Z

This helps, but the CJS types are still wrong

I thought so at first as well, but it seems that if I set the module option to CommonJS the same output is produced. Even if I use tsc manually with a config that only compiles CommonJS I get the same result:

{
  "files": [
    "./lib/index.ts"
  ],
  "compilerOptions": {
    "target": "ES5",
    "module": "CommonJS",
    "strict": true,
    "declaration": true,
    "emitDeclarationOnly": true,
    "declarationDir": "./typings",
  }
}

It seems like this is just how the TypeScript compiler wants to emit types for the CommonJS? Perhaps I am missing something, can you manage to get the output you expect with tsc?

The simplest solution is to simply not use default exports, but named exports.

Totally agree, but that would be a breaking change. And @frederikprijck has made it clear that is not an option for him.

Also I believe tsup can just replace rollup here.

It could, but we can't do this because:

It does not support bundling UMD.
It cannot have mixed default and named exports for CommonJS, and neither can Rollup (which is why we have this hack).

My personal opinion aligns with you, which would be:

Do a major version bump for breaking changes.
Use named exports only, this avoids the aforementioned mixed exports issue.
Drop the UMD bundle (and possibly also CommonJS since we're doing a breaking change anyways)
Simplify tooling by dropping Rollup in favor of tsup.

But again, that would be a breaking change, which I don't believe is an option.

frederikprijck · 2023-07-31T16:58:57Z

Hey, i want to mention that introducing a breaking change for this is perfectly fine, but i would avoid it if not necessary. But it looks like it will be neccesary and i have no objections, sorry if it may have sounded like that before. Personally, i prefer to avoid any default export myself, so i am happy to do so inhere if it aids with our setup.

What i do not want is drop commonjs, for reasons mentioned before.

Regarding tsup, all our JavaScript SDKs (we have alot) are using rollup, so for several reasons i believe we should stick to it, unless there is a clear reason we can not.

So let's go with dropping default exports, but keep commonjs and rollup around.

cristobal · 2023-07-31T17:16:08Z

Hmm @frederikprijck why is main set to build/cjs/jwt-decode.js, i think this could be a bit problematic when the type is module set in package.json.

Why not use the exports field instead?

frederikprijck · 2023-07-31T17:18:39Z

@cristobal can you elaborate on what's going wrong, probably outside of this PR?

Not all tooling relies on exports, so we might need to support both.

jonkoops · 2023-07-31T17:22:25Z

But it looks like it will be necessary and i have no objections, sorry if it may have sounded like that before.

@frederikprijck did not mean to imply you were holding this up or anything, just that it was a concern for backwards compatibility. Sorry if it came across as such. Some maintainers can be a bit more strict about backwards compatibility 😛

What i do not want is drop commonjs, for reasons mentioned before.

Yeah that should not be an issue, still complicates things slightly, but not as much if we decide to drop the default export. What is your take on the UMD module though? I'd personally prefer to get rid of it, but I understand if you feel differently.

Regarding tsup, all our JavaScript SDKs (we have alot) are using rollup, so for several reasons i believe we should stick to it, unless there is a clear reason we can not.

There is a couple of reasons I can think of, but the main one would be that it already does everything we need (except for UMD), and that pretty much makes Rollup obsolete. It also follows all the best practices and could replace the config we have with a single line script entry in the package.

If you like, I can create a draft PR to show you what this could look like under ideal circumstances. Just to get an idea.

So let's go with dropping default exports, but keep commonjs and rollup around.

Got it, I'll make a separate PR for that so we can land this quickly and without to many files to review.

jonkoops · 2023-07-31T17:23:53Z

Why not use the exports field instead?

We already do, and this has not changed in this PR. I've just moved the code around a little. And as @frederikprijck mentioned, there is tooling out there that does not yet understand the exports field, so we keep main and module around for backwards compatibility.

frederikprijck · 2023-07-31T17:28:26Z

@jonkoops ,

I opened a PR (#175 ) to avoid default exports.

What is your take on the UMD module though? I'd personally prefer to get rid of it, but I understand if you feel differently.

How would we support people currently relying on the UMD bundle, mostly by including the script tag in their HTML file?

cristobal · 2023-07-31T17:28:41Z

This helps, but the CJS types are still wrong (ATTW is nice, but not yet perfect). They should not define export default jwtDecode, but:
declare function jwtDecode(/* … */) // …


declare namespace jwtDecode {
  class InvalidTokenError extends Error {
    // …
  }
}

export = jwtDecode
The simplest solution is to simply not use default exports, but named exports.

Also I believe tsup can just replace rollup here.

This is what also rollup recommends, which is used for bundling here
https://rollupjs.org/configuration-options/#output-exports

cristobal · 2023-07-31T17:30:01Z

@jonkoops ,

I opened a PR (#175 ) to avoid default exports.

What is your take on the UMD module though? I'd personally prefer to get rid of it, but I understand if you feel differently.

How would we support people currently relying on the UMD bundle, mostly by including the script tag in their HTML file?

I'm not sure why you need to have an UMD bundle here, you can add it in the README that people relying on UMD can rather bundle it themselves or stick with the old version.

jonkoops · 2023-07-31T17:55:16Z

How would we support people currently relying on the UMD bundle, mostly by including the script tag in their HTML file?

Well, by supporting ESM we can support them. They can import it as they would other modules:

<script type="module">
  import { jwtDecode } from "/path-to-esm/jwt-decode.js";
  
  const token = "eyJ0eXAiO.../// jwt token";
  const decoded = jwtDecode(token);
</script>

Or alternatively, if an import map is used, this would look the same as ESM and Node.js:

<script type="module">
  import { jwtDecode } from "jwt-decode";
</script>

cristobal · 2023-07-31T18:00:36Z

How would we support people currently relying on the UMD bundle, mostly by including the script tag in their HTML file?

Well, by supporting ESM we can support them. They can import it as they would other modules:
<script type="module">
  import { jwtDecode } from "/path-to-esm/jwt-decode.js";
  
  const token = "eyJ0eXAiO.../// jwt token";
  const decoded = jwtDecode(token);
</script>
Or alternatively, if an import map is used, this would look the same as ESM and Node.js:
<script type="module">
  import { jwtDecode } from "jwt-decode";
</script>

Again as you say by supporting esm we can support them, however there is no need for an UMD js file then?

This is more of an Documentation issue rather than anything else, or am i missing something?

jonkoops · 2023-07-31T18:02:46Z

@cristobal I recommend you read my comments in this thread, I am not advocating for UMD.

cristobal · 2023-07-31T18:05:34Z

@cristobal I recommend you read my comments in this thread, I am not advocating for UMD.

@jonkoops my question about UMD was directed to @frederikprijck here.

jonkoops · 2023-07-31T18:31:54Z

Ok, got confused there since you are quoting my post.

frederikprijck · 2023-07-31T18:40:28Z

I have a feeling you both have a strong reason to drop it, while i believe don't see the issue.

If so, can you please open an issue so we can have a central place that elaborates on the issues with keeping it.

Please refrain from any personal opinion about wether or not we should support it, but elaborate on the reasons we should drop it beyond personal preference.

My opinion is to keep it, even if there are alternative paths for those users to migrate. I want to avoid unneccesarily breaking users beyond things that are needed, especially given this lib is used extensively.

remcohaszing · 2023-07-31T19:16:39Z

@jonkoops

It seems like this is just how the TypeScript compiler wants to emit types for the CommonJS?

Yes. This is correct. This results in the following build output:

exports.default = jwtDecode

Perhaps I am missing something, can you manage to get the output you expect with tsc?

Yes. TypeScript isn’t intended for dual publishing. It also can’t, because CJS and ESM are incompatible, especially default exports. To get proper output, you need both CJS and ESM source files.

Named exports make it simpler though. Because now this:

export function jwtDecode() {}

can be compiled to:

function jwtDecode() {}

exports.jwtDecode = jwtDecode

The problem with Rollup and other third party TypeScript build tools is, they understand JavaScript, but not TypeScript. Aside from tsc, tsup is the only tool that does this somewhat correctly (though it can’t transform default exports as desirable).

I doubt if people actually still use UMD. However, rollup could still be used to produce only the UMD bundle.

jonkoops · 2023-07-31T20:13:38Z

I agree with @frederikprijck, let's move the UMD discussion to a separate issue. I think it's just cluttering up the discussion here. I've created #180 to continue this discussion.

jonkoops · 2023-07-31T20:15:55Z

Yes. TypeScript isn’t intended for dual publishing. It also can’t, because CJS and ESM are incompatible, especially default exports. To get proper output, you need both CJS and ESM source files.

Sigh, dual packaging is like the gift bag of needles that never stops giving. Any idea how we can accomplish this without creating new source code?

jonkoops · 2023-08-01T13:23:14Z

@remcohaszing I have not yet found a way to get the TypeScript compiler to emit correct types for CommonJS. Perhaps this is the best we can do for now. The types are already linked the same way for v3, and I have seen no complaints. Could be that the combination of CommonJS and TypeScript is just not that popular?

Side-note: I've found a similar issue in Deno's DNT (denoland/dnt#327).

remcohaszing · 2023-08-01T13:44:50Z

The generated types are correct when using a named export. @frederikprijck mentioned this is ok in #174 (comment).

jonkoops · 2023-08-01T13:54:05Z

The generated types are correct when using a named export.

I am not sure it is correct. When I rebase this PR on the work in #175, I am still getting ESM-style exports:

export { InvalidTokenError, JwtDecodeOptions, JwtHeader, JwtPayload, jwtDecode };

remcohaszing · 2023-08-01T14:05:09Z

This looks fine.

The following CJS export style:

exports.InvalidTokenError = InvalidTokenError;
exports.jwtDecode = jwtDecode;

requires the following type definitions:

export {
  InvalidTokenError,
  jwtDecode
}

However, the following CJS exports style:

module.exports = something;

Requires the following type definitions:

export = something;

This is why named exports solve many ESM/CJS compatibility issues for dual publishing.

jonkoops · 2023-08-01T14:14:09Z

@remcohaszing I'll take your word for it, but I was kind of expecting something like this:

export = { jwtDecode };

Is there some kind of reference for this? I can't find anything in the TypeScript documentation.

remcohaszing · 2023-08-01T14:28:25Z

There is some documentation in https://www.typescriptlang.org/docs/handbook/modules.html and https://www.typescriptlang.org/docs/handbook/esm-node.html, but IMO the explanation isn’t great. This draft document explains a lot more in depth, but it’s a lot to read and not finished.

Really the best thing you can do is fiddle with a simple project:

// tsconfig.json
{
  "compilerOptions": {
    "declaration": true,
    "module": "node16"
  }
}

// foo.cts
export interface Options {}

export function foo(options: Options) {}

// bar.cts
declare namespace bar {
  interface Options {}
}

function bar(options: bar.Options) {}

export = bar

and then fiddle yourself. See what the output is when running tsc

jonkoops · 2023-08-01T15:51:03Z

@remcohaszing Yeah, I am getting the same. Let's consider this resolved then 👍

jonkoops · 2023-08-01T15:52:50Z

Just did a quick rebase so the changes from #175 are included, so I think this is good to go. @remcohaszing @frederikprijck would you take another look and let me know if things look good?

frederikprijck · 2023-08-02T10:24:59Z

package.json

@@ -17,8 +30,10 @@
  "homepage": "https://github.com/auth0/jwt-decode#readme",
  "scripts": {
    "dev": "rollup --sourcemap --config --configPlugin typescript",
-    "build": "rimraf build && rollup --sourcemap --config --configPlugin typescript --environment NODE_ENV:production",
-    "postbuild": "echo '{\"type\": \"commonjs\"}'> build/cjs/package.json",


Do we no longer need this?

Now that we're using the .cjs extension this should work without an additional package file.

frederikprijck

Thanks for the work here, this looks great.

remcohaszing · 2023-08-03T09:27:35Z

Yes, this works great! Thanks!

jonkoops · 2023-08-03T10:54:01Z

Thanks for getting this landed all!

jonkoops requested a review from a team as a code owner July 31, 2023 15:25

jonkoops commented Jul 31, 2023

View reviewed changes

jonkoops mentioned this pull request Jul 31, 2023

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

Closed

4 tasks

frederikprijck mentioned this pull request Jul 31, 2023

Avoid using default exports #175

Merged

3 tasks

jonkoops mentioned this pull request Jul 31, 2023

Consider dropping Universal Module Definition (UMD) bundle #180

Closed

4 tasks

Ensure types are bundled and correctly linked

fc6ec3f

jonkoops force-pushed the fix-types branch from 7f6d52f to fc6ec3f Compare August 1, 2023 15:50

frederikprijck mentioned this pull request Aug 1, 2023

Generate single typings directory (BETA) #185

Closed

frederikprijck reviewed Aug 2, 2023

View reviewed changes

jonkoops requested a review from frederikprijck August 2, 2023 16:59

frederikprijck approved these changes Aug 2, 2023

View reviewed changes

frederikprijck merged commit 40b68ed into auth0:beta Aug 2, 2023

jonkoops deleted the fix-types branch August 3, 2023 10:53

frederikprijck mentioned this pull request Aug 3, 2023

Release v4.0.0-beta.2 #186

Merged

frederikprijck pushed a commit that referenced this pull request Oct 27, 2023

Ensure types are bundled and correctly linked (#174)

c05d0ac

rpt-uk-github mentioned this pull request Dec 12, 2023

[Snyk] Upgrade jwt-decode from 3.1.2 to 4.0.0 reapit/foundations#10411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure types are bundled and correctly linked #174

Ensure types are bundled and correctly linked #174

jonkoops commented Jul 31, 2023

jonkoops commented Jul 31, 2023

jonkoops Jul 31, 2023

remcohaszing commented Jul 31, 2023

jonkoops commented Jul 31, 2023

frederikprijck commented Jul 31, 2023 •

edited

Loading

cristobal commented Jul 31, 2023

frederikprijck commented Jul 31, 2023 •

edited

Loading

jonkoops commented Jul 31, 2023

jonkoops commented Jul 31, 2023

frederikprijck commented Jul 31, 2023

cristobal commented Jul 31, 2023

cristobal commented Jul 31, 2023

jonkoops commented Jul 31, 2023

cristobal commented Jul 31, 2023

jonkoops commented Jul 31, 2023

cristobal commented Jul 31, 2023 •

edited

Loading

jonkoops commented Jul 31, 2023

frederikprijck commented Jul 31, 2023 •

edited

Loading

remcohaszing commented Jul 31, 2023

jonkoops commented Jul 31, 2023

jonkoops commented Jul 31, 2023

jonkoops commented Aug 1, 2023

remcohaszing commented Aug 1, 2023

jonkoops commented Aug 1, 2023

remcohaszing commented Aug 1, 2023

jonkoops commented Aug 1, 2023

remcohaszing commented Aug 1, 2023

jonkoops commented Aug 1, 2023

jonkoops commented Aug 1, 2023

frederikprijck Aug 2, 2023

jonkoops Aug 2, 2023 •

edited

Loading

frederikprijck left a comment

remcohaszing commented Aug 3, 2023

jonkoops commented Aug 3, 2023

Ensure types are bundled and correctly linked #174

Ensure types are bundled and correctly linked #174

Conversation

jonkoops commented Jul 31, 2023

jonkoops commented Jul 31, 2023

jonkoops Jul 31, 2023

Choose a reason for hiding this comment

remcohaszing commented Jul 31, 2023

jonkoops commented Jul 31, 2023

frederikprijck commented Jul 31, 2023 • edited Loading

cristobal commented Jul 31, 2023

frederikprijck commented Jul 31, 2023 • edited Loading

jonkoops commented Jul 31, 2023

jonkoops commented Jul 31, 2023

frederikprijck commented Jul 31, 2023

cristobal commented Jul 31, 2023

cristobal commented Jul 31, 2023

jonkoops commented Jul 31, 2023

cristobal commented Jul 31, 2023

jonkoops commented Jul 31, 2023

cristobal commented Jul 31, 2023 • edited Loading

jonkoops commented Jul 31, 2023

frederikprijck commented Jul 31, 2023 • edited Loading

remcohaszing commented Jul 31, 2023

jonkoops commented Jul 31, 2023

jonkoops commented Jul 31, 2023

jonkoops commented Aug 1, 2023

remcohaszing commented Aug 1, 2023

jonkoops commented Aug 1, 2023

remcohaszing commented Aug 1, 2023

jonkoops commented Aug 1, 2023

remcohaszing commented Aug 1, 2023

jonkoops commented Aug 1, 2023

jonkoops commented Aug 1, 2023

frederikprijck Aug 2, 2023

Choose a reason for hiding this comment

jonkoops Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

frederikprijck left a comment

Choose a reason for hiding this comment

remcohaszing commented Aug 3, 2023

jonkoops commented Aug 3, 2023

frederikprijck commented Jul 31, 2023 •

edited

Loading

frederikprijck commented Jul 31, 2023 •

edited

Loading

cristobal commented Jul 31, 2023 •

edited

Loading

frederikprijck commented Jul 31, 2023 •

edited

Loading

jonkoops Aug 2, 2023 •

edited

Loading