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

feature: Requesting support for TS ESM/ES6 import #1442

Open
kf6kjg opened this issue Apr 6, 2023 · 23 comments
Open

feature: Requesting support for TS ESM/ES6 import #1442

kf6kjg opened this issue Apr 6, 2023 · 23 comments
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request

Comments

@kf6kjg
Copy link

kf6kjg commented Apr 6, 2023

Description

Your automatic reaction will be "but we already DO" - however the current setup of this repository, even the setup in PR #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
Copy link
Contributor

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
Copy link
Author

kf6kjg commented Apr 6, 2023

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
Copy link
Contributor

@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:
immagine_2023-04-06_230040193

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

@carlocorradini
Copy link
Contributor

@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
Copy link
Author

kf6kjg commented Apr 6, 2023

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
Copy link
Contributor

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

@carlocorradini
Copy link
Contributor

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

@kf6kjg
Copy link
Author

kf6kjg commented Apr 6, 2023

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.

kf6kjg added a commit to kf6kjg/type-graphql that referenced this issue Apr 7, 2023
Took some doing since there's no nice way in TS of omitting comments from the resulting JS files but keeping them in the TS files. See microsoft/TypeScript#14619 for the relevant issue discussing this.

Fixes MichalLytek#1442
kf6kjg added a commit to kf6kjg/type-graphql that referenced this issue Apr 7, 2023
Took some doing since there's no nice way in TS of omitting comments from the resulting JS files but keeping them in the TS files. See microsoft/TypeScript#14619 for the relevant issue discussing this.

Fixes MichalLytek#1442
@carlocorradini
Copy link
Contributor

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

@carlocorradini
Copy link
Contributor

I'll wait an update 🥳🤗

kf6kjg added a commit to kf6kjg/type-graphql that referenced this issue Apr 10, 2023
Took some doing since there's no nice way in TS of omitting comments from the resulting JS files but keeping them in the TS files. See microsoft/TypeScript#14619 for the relevant issue discussing this.

Fixes MichalLytek#1442
@kf6kjg
Copy link
Author

kf6kjg commented Apr 10, 2023

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
Copy link
Owner

I think this can be closed by #1400 🔒

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

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved labels Aug 9, 2023
@kf6kjg
Copy link
Author

kf6kjg commented Aug 9, 2023

@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.

@MichalLytek MichalLytek removed the Solved ✔️ The issue has been solved label Aug 9, 2023
@carlocorradini
Copy link
Contributor

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
Copy link
Contributor

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

@carlocorradini
Copy link
Contributor

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
Copy link
Author

kf6kjg commented Aug 14, 2023

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
Copy link
Contributor

@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
Copy link
Owner

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants
@kf6kjg @MichalLytek @itpropro @carlocorradini and others