auth0 / node-auth0

Node.js client library for the Auth0 platform.
MIT License
628 stars 307 forks source link

Expose typescript definitions only once #952

Closed alaczi closed 11 months ago

alaczi commented 11 months ago

Checklist

Describe the problem you'd like to have solved

Right now type definitios are exported in multiple files and it confuses the IDE. For example these import are all valid:

import { UsersManager } from 'auth0'
import { UsersManager } from 'auth0/dist/cjs/management'
import { UsersManager } from 'auth0/dist/cjs/management/__generated/managers'
import { UsersManager } from 'auth0/dist/esm/management'
import { UsersManager } from "auth0/dist/esm/management/__generated/managers";

Describe the ideal solution

Expose the definitions only once

Alternatives and current workarounds

No response

Additional context

No response

frederikprijck commented 11 months ago

Thanks for reaching out. The different exports of the types is expected and used to support both CJS and ESM, but you shouldnt import them directly like you are showing (even more so, tooling should block you from doing that).

I tried this in a simple project, and all of the above imports are (correctly) marked invalid (apart from the first one):

image

Can you please share something that reproduces this so we can look into what's going on?

Please also provide all the information that's requested in the issue template so we know what version to look at etc when investigating., currently it's missing things like the SDK version as well as the version of Node being used. Additionally it makes sense to share what tools are involved (webpack, rollup, ...). However, it would realy help us if you would create a minimal reproduction of the setup you have for us to look at.

alaczi commented 11 months ago

@frederikprijck I used this in an existing project, will put together a new empty one and check with it. Will get back to you.

alaczi commented 11 months ago

@frederikprijck I use IntelliJ Phpstorm as my IDE

I tried this on a bare project. 1, I used this guide to set up a new typescript projec: https://www.digitalocean.com/community/tutorials/typescript-new-project 2, added the sample code:

import { UsersManager } from 'auth0'
import { UsersManager as UsersManager2} from 'auth0/dist/cjs/management'
import { UsersManager as UsersManager3} from 'auth0/dist/cjs/management/__generated/managers'
import { UsersManager as UsersManager4 } from 'auth0/dist/esm/management'
import { UsersManager as UsersManager5} from "auth0/dist/esm/management/__generated/managers";

const manager1 = new UsersManager({} as any);
const manager2 = new UsersManager2({} as any);
const manager3 = new UsersManager3({} as any);
const manager4 = new UsersManager4({} as any);
const manager5 = new UsersManager5({} as any);

3, ran npx tsc the file was compiled without any errors.

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const auth0_1 = require("auth0");
const management_1 = require("auth0/dist/cjs/management");
const managers_1 = require("auth0/dist/cjs/management/__generated/managers");
const management_2 = require("auth0/dist/esm/management");
const managers_2 = require("auth0/dist/esm/management/__generated/managers");
const manager1 = new auth0_1.UsersManager({});
const manager2 = new management_1.UsersManager({});
const manager3 = new managers_1.UsersManager({});
const manager4 = new management_2.UsersManager({});
const manager5 = new managers_2.UsersManager({});

4, the IDE does not give me any warning about the wrong imports Screenshot 2023-10-11 064146

I think the problem is that the types are generated and exported twice, but generally types don't really depend on the actual module system used in the resulted js after compilation.

So instead this in package json

  "types": "dist/cjs/index.d.ts",
  "type": "module",
  "exports": {
    ".": {
      "import": {
        "types": "./dist/esm/index.d.ts",
        "default": "./dist/esm/index.js"
      },
      "require": {
        "types": "./dist/cjs/index.d.ts",
        "default": "./dist/cjs/index.js"
      }
    }
  },

maybe generating the types once and exporting them would be better:

  "exports": {
    "./*": {
      "types": "./build/types/*.d.ts",
      "require": "./build/cjs/*.js",
      "import": "./build/esm/*.js",
      "default": "./build/esm/*.js"
    }
  }

Note: I haven't tried the one above but this guide looks comprehensive how to build a typescript project for both esm and cjs: https://thesametech.com/how-to-build-typescript-project/

frederikprijck commented 11 months ago

Thanks for that context, can you try setting "moduleResolution": "node16" in your tsconfig and see how it behaves differently?

As you can see per https://www.typescriptlang.org/tsconfig#moduleResolution, node16 is the value you most likely want to use and it supports our package.json's exports fields, which seems to behave correctly.

I can see that, when omitting any moduleResolution, it defaults to node10, which is called out on the above link to be something you probably do not want for modern projects. That's why we can see a difference between the two.

Would setting it to node16 be possible for your project?

Based on the following tools it looks like our types are configured correctly, and what you are seeing may be a downside of using an older moduleResolution on a library that supports more modern versions as well:

Just to understand correctly, everything works as expected, your IDE is just having difficulties auto-resolving a modern project in a project using an older moduleResolution, is that accurate (I know you are using the default value, but it's considered an older one and as mentioned above called out something u dont want to use for modern projects)?

Regardless, i will have a look if anything can be improved here.

frederikprijck commented 11 months ago

I just want to update that I tried everything that's explained in https://thesametech.com/how-to-build-typescript-project/ (admittedly, this is what we had before and moved away from), and arethetypeswrong complains about incorrectly configured types for the project.

image

The error is: Import resolved to an ESM type declaration file, but a CommonJS JavaScript file. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md

As you can see in that link:

This problem usually happens when a library that includes both a CJS and ESM implementation attempts to use a single .d.ts file to represent both

So we should not have a single type declarations file, and the solution mentioned there ies exactly what we are already doing.

I would argue that, if it isn't causing any issues aside from confusing the IDE, we want to leave things as is and see if you can move to Node16 as the module resolution.

alaczi commented 11 months ago

@frederikprijck Thanks for the quick answers. I can confirm, that with

    "module": "node16",
    "moduleResolution": "node16",

it won't see the module specific definitions and tsc will throw an error.

with

    "module": "commonjs",
    "moduleResolution": "node",

you can import the module specific definitions and the confusion persists πŸ˜„

But if you use node16 some of the types are not exposed for example the ApiResponse. Is that deliberate?

const user: ApiResponse<GetUsers200ResponseOneOfInner> = await manager.get({id: "a"});

Also I had some issues with typesafe jest mocks, but I will need to create some examples to see if I can reproduce it.

frederikprijck commented 11 months ago

But if you use node16 some of the types are not exposed for example the ApiResponse. Is that deliberate?

Based on my testing, this is not related to node16. And this is not deliberate, fixed with https://github.com/auth0/node-auth0/pull/954.

Thank you for all the reports, it's truly helpful πŸ‘

frederikprijck commented 11 months ago

Having done some investigation, I am going to close this as I think there isnt anything we can change here to improve when using the old node10 moduleresolution. Everything works, but the fact that the IDE is confused is a consequence of us supporting the modern alternatives.