feature: Requesting support for TS ESM/ES6 import

[kf6kjg]

Description

Your automatic reaction will be "but we already DO" - however the current setup of this repository, even the setup in PR https://github.com/MichalLytek/type-graphql/pull/1400, has an error.

When using this library in my project which has the package.json setting type set to module the TS compiler throws errors throws the following when internally importing the ESM-capable library class-validator:

 ../../node_modules/type-graphql/dist/errors/ArgumentValidationError.d.ts(1,38): 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("class-validator")' call instead.

I believe that this is happening because the contents of type-graphql's typings folder is being interpreted by TS as CommonJS instead of ESM, even though this library exports ESM Javascript in another folder.

Note that I'm using TypeScript 4.9.5 against Node v18.14.2.

Proposed solution

There are a couple ways to solve this that I can see, each with its own negatives:

1. Place "type": "module" in the root package.json and abandon CommonJS. Probably not what the author even wants to get anywhere near.
2. During the build process place a package.json containing {"type": "module"} in the typings folder. This has the negative consequence that anyone who is using TypeScript while targeting CommonJS will have an error because CommonJS cannot require ESM.
3. Eliminate the typings folder in favor of placing the types with the ESM and CJS source code and altering the root package.json as follows . The main negative here is that the types are duplicated and that ESM users have to change their import path to import { ... } from "type-graphql/esm".

   {
     "exports": {
       ".": {
         "require": "./build/cjs/index.js"
       },
       "./esm": {
         "import": "./build/esm/index.js"
       },
     },
     "main": "./build/cjs/index.js",
     "module": "./build/esm/index.js",
   }
   

   Please note that this solution is theoretical - I've not checked that it is entirely valid. There's a comment that I'm relying upon that indicates that if the .js and .d.ts files are kept together no extra type fields in the package.json are needed.

[carlocorradini]

So, also typings requires a package.json with type: module? I have to check this :/

If this is the case, we need to duplicate the typings and all should be resolved.

[kf6kjg]

I just realized that yes there's a possible alternate to the 3rd solution that simplifies:

{
     "exports": {
       ".": {
         "require": "./build/cjs/index.js",
         "import": "./build/esm/index.js"
       },
     },
     "main": "./build/cjs/index.js",
     "module": "./build/esm/index.js",
}

AKA simply omitting the types fields.

[carlocorradini]

@kf6kjg I've tried now and it is working flawlessy: package.json:

{
    // ...
    "type": "module"
}

tsconfig.json:

{
    // ...
    "compilerOptions": {
        // ...
        "module": "esnext"
    }
}

If you do all correctly you will see 2 errors:

1) We need to update require(...) to be ESM compatible 2) Check package.json 2 or 3 level up

[carlocorradini]

@kf6kjg This is the current package.json configuration:

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

[kf6kjg]

ESM code should NEVER contain require(...). Use await import(...) instead, or use a full import {...} from '...'.

So I did the following:

1. Edited as per the attached patch file based off your branch: kf6kjg.2023040601.patch
2. Ran the following commands:

$ npm run build

> [email protected] prebuild
> npm run clean


> [email protected] clean
> npx npm-run-all --npm-path npm "clean:*"


> [email protected] clean:build
> npx shx rm -rf build


> [email protected] clean:coverage
> npx shx rm -rf coverage


> [email protected] build
> npx tsc --build ./tsconfig.cjs.json ./tsconfig.esm.json


> [email protected] postbuild
> npx ts-node ./scripts/package.json.ts

$ cd build/esm 
$ node index.js # Exit code was 0.
$ cd ../cjs
$ node index.js # Exit code was 0.

EDIT: Just realized what you were doing in the examples folder. Exploring that space...

[carlocorradini]

@kf6kjg I know we are working on it. Is still a PR :)

[carlocorradini]

@kf6kjg Example are currently broken due to graphql -> We need to migrate to a monorepo for them :/

[kf6kjg]

I've been introducing the place where I work to NX-based monorepo setups. It's been fun.

Additionally in my working copy of #1400 I got to that same point where node complains about "Duplicate "graphql" modules cannot be used at the same time". This is a side effect of putting the examples in the same repository without using a monorepo. Whee, feel you there.

[carlocorradini]

@kf6kjg With the latest update you should have no problems now :)

[carlocorradini]

I'll wait an update 🥳🤗

[kf6kjg]

That was a LOT of digging. I've finally created a reproduction case. Be sure to read the README located in the patches folder to see the reason for each of the patches. These are not recommended to be included in type-graphql - they'd cause other problems. However they allow you to check a pure ESM import tree which type-graphql currently does not have - at least as pure as I've gotten it to be.

If it weren't for libphonenumber-js's node16 TypeScript under CJS error this would have been totally invisible.

This also leads to a workaround: set "skipLibCheck": true in any consumers of type-graphql and pray that there's no trouble.

[MichalLytek]

I think this can be closed by #1400 🔒

@carlocorradini Can you share your example repo with ESM usage?

[kf6kjg]

@MichalLytek I need to verify, but I expect the same underlying problem still exists. To my knowledge the root cause has not been resolved; only masked via setting "skipLibCheck": true - something I'd prefer to not use ASAP.

[carlocorradini]

mhmhmhm I've tried skipLibCheck: false but it just results in too many problems that are solely dependent issues😅 I agree that it should be set to false but for every library we need to create PR to fix their errors (i.e. @types/shelljs fails due to glob option)

[carlocorradini]

If you want to fix all third-party issue(s) the challenge is yours 😅🤯

[carlocorradini]

I think this can be closed by #1400 🔒

@carlocorradini Can you share your example repo with ESM usage?

This issue can be closed. @kf6kjg Can you create a new issue for skipLibCheck? @MichalLytek An example using ESM is really necessary?

[kf6kjg]

I still think that neither of you understand the core of this issue. And that's me failing to communicate clearly.

This issue is not about the sate of skipLibCheck in this repo. It is not about issues in 3rd party tools.

It is 100% about that fact that TypeScript dual-mode packages, such as this one, have very specific rules and that this package is using a shortcut that results in its ESM exports being ignored by TypeScript and brought into the compiler via the CommonJS compatibility system.

In my OP I gave 3 recommendations. Back then option 3 was theoretical. Since then I've had to set up some pure ESM and pure CJS projects that brought in a dual-mode ESM+CJS library I created. To make that work I had to have types compiled into the JS tree: one set of types for CJS, one for ESM. To do otherwise caused either the pure CJS or the pure ESM package to fail to compile. If you need I can build proofs of concept that you can try to import into.

[carlocorradini]

@kf6kjg UNDERSTOOD 🤯🤯🤯

Personally I prefer the first option where we set type: module in the root (Node.js > =16, therefore ESM is supported). We need to wait @MichalLytek because I know he doesn't like import with extension (import { a } from "./b.js";) and there is the need to do some refactoring (i.e. scripts) since __dirname is no more available.

[MichalLytek]

@carlocorradini I was thinking about exploring the option 3

In my OP I gave 3 recommendations. Back then option 3 was theoretical. Since then I've had to set up some pure ESM and pure CJS projects that brought in a dual-mode ESM+CJS library I created. To make that work I had to have types compiled into the JS tree: one set of types for CJS, one for ESM. To do otherwise caused either the pure CJS or the pure ESM package to fail to compile. If you need I can build proofs of concept that you can try to import into.

[carlocorradini]

I think that if we specify the paths to the corresponding types (ESM and CJS) it should work without /esm (it's ugly and not portable)

[carlocorradini]

I think that if we specify the paths to the corresponding types (ESM and CJS) it should work without /esm (it's ugly and not portable)

Ok it doesn't work :(

[itpropro]

Any updates on this? A clean, tested and working ESM module would already fix many issues in current and future usage of type-graphql. It would also help to clean up the code base to remove packages that have strange or not esm/cjs compliant exports. Maybe it's worth checking out something like https://github.com/unjs/unbuild for building, which uses rollup and esbuild.

[carlocorradini]

The simplest way, in my opinion, is to just delete the typings directory and duplicate the declarations