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

Incorrect ESM build #10

Open
Cellule opened this issue Aug 4, 2024 · 11 comments · Fixed by #11 or #13
Open

Incorrect ESM build #10

Cellule opened this issue Aug 4, 2024 · 11 comments · Fixed by #11 or #13
Assignees
Labels
bug Something isn't working

Comments

@Cellule
Copy link

Cellule commented Aug 4, 2024

There are a few issues with the current ESM build output
See example of errors here https://codesandbox.io/p/devbox/confident-orla-xy9d9n?file=%2Findex.js

At runtime it seems it's not loading the .es.js version of the package
If I add

// package.json
{
  "exports":{
    ".": {
      "import": "./dist/rand-seed.es.js",
      "require": "./dist/rand-seed.js",
      "types": "./dist/index.d.ts"
    }
  },
}

Then it correctly loads rand-seed.es.js. However since the package is commonjs, I then get other errors.
Would need to isolate rand-seed.es.js in another folder and add another package.json with just { "type": "module" } in it.

Plus in typescript I get the following error
image

I'm not sure how to fix all of this with rollup, so far I've only done dual commonjs/esm build with only typescript
Here's what I use for instance where the package is primarily ESM then I isolate the commonjs build

// package.json
{
  "type": "module",
  "scripts": {
    "build": "yarn build:esm && yarn build:cjs",
    "build:esm": "yarn tsc --build",
    "build:cjs": "yarn tsc --build tsconfig.cjs.json && echo '{ \"type\": \"commonjs\" }' > dist-cjs/package.json"
  },
  "exports": {
    ".": {
      "types": {
        "import": "./dist/index.d.ts",
        "require": "./dist-cjs/index.d.ts"
      },
      "import": "./dist/index.js",
      "require": "./dist-cjs/index.js",
      "default": "./dist/index.js"
    }
  },
  "main": "./dist-cjs/index.js",
  "module": "./dist/index.js",
  "types": "./dist/index.d.ts",
  "files": [
    "dist",
    "dist-cjs",
    "package.json",
    "README.md",
    "CHANGELOG.md"
  ],
}
// tsconfig.cjs.json
{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "tsBuildInfoFile": ".cache/.cjs.tsbuildinfo",
    "module": "commonjs",
    "moduleResolution": "node",
    "outDir": "dist-cjs",
  }
}
@michaeldzjap
Copy link
Owner

michaeldzjap commented Aug 5, 2024

Thanks for reporting. I think it might have to do with the recent changes regarding importing JSON in the rollup config. I should have tested this better. I will try some things out.

@michaeldzjap michaeldzjap self-assigned this Aug 5, 2024
@michaeldzjap michaeldzjap added the bug Something isn't working label Aug 5, 2024
@michaeldzjap michaeldzjap linked a pull request Aug 10, 2024 that will close this issue
@michaeldzjap
Copy link
Owner

michaeldzjap commented Aug 10, 2024

@Cellule This should be fixed now (hopefully) in v2.1.0.

@Cellule
Copy link
Author

Cellule commented Aug 10, 2024

I feel you are so close!
I updated my repro https://codesandbox.io/p/devbox/confident-orla-xy9d9n with typescript ESM & CommonJS using 2.1.0
Right now I get error Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /project/sandbox/node_modules/rand-seed/package.json imported from /project/sandbox/index.ts
Also typescript build still fails

So far the way I managed to make it work for both commonjs and ESM in both javascript and typescript, you need to output 2 dist folders
Keep the one you currently have as the CommonJS build
Then output another folder, I used dist-es, with roughly the same exact content BUT add a package.json file with { "type": "module" } in it, this will ensure Typescript correctly understands the typings in that other folder

Here's what the root package.json could look like

{
  "exports": {
    ".": {
      "types": {
        "import": "./dist-es/index.d.ts",
        "require": "./dist/index.d.ts"
      },
      "import": "./dist-es/rand-seed.mjs",
      "require": "./dist/rand-seed.js",
      "default": "./dist-es/rand-seed.mjs"
    }
  },
  "main": "dist/rand-seed.js",
  "module": "dist-es/rand-seed.mjs",
  "files": [ "dist", "dist-es" ],
  "types": "dist/index.d.ts",
}

I recommend you test your changes in a fork of my repro, it should be a good indicator if you got it working

@michaeldzjap michaeldzjap reopened this Aug 11, 2024
@michaeldzjap
Copy link
Owner

What a headache this is... Anyway, I think I've finally managed to solve it now. I tried the latest release in your codesandbox repro and everything seems to run / build now.

Please let me know if there's still something not working for you and thanks for the help with this @Cellule!

@Cellule
Copy link
Author

Cellule commented Aug 11, 2024

I agree, this is a horrible nightmare that we can't seem to wake up from

I feel you went 1 step in the right direction by converting to ESM first.
However, now the commonjs typescript build is broken

Here's what I think can solve this once and for all (I hope)

  1. In the output dist/index.d.ts the import must have .js because it is a requirement of ESM: import Rand, { PRNG } from './Rand.js';
  2. You need to tell typescript to load types from a "commonjs" output. Here's how I achieved this as simple as I can without adding more build steps
  • Create a folder dist-cjs with files index.d.ts and package.json
// dist-cjs/index.d.ts

// @ts-expect-error -- ESM/CJS warning
export * from "../dist/index.js"
// @ts-expect-error -- ESM/CJS warning
export {default} from "../dist/index.js"

// ^ This should tell typescript to shut up about the "incompatibility" error where a commonjs file is trying to import an ESM one
// dist-cjs/package.json
{ "type": "commonjs" }
// ^ This overrides the default type of the package for the index.d.ts file in dist-cjs
  • Commit and include the dist-cjs folder in the "files" on the root package.json
  • Update root package.json to the following
{
  "exports": {
    ".": {
      "import": {
        "types": "./dist/index.d.ts",
        "default": "./dist/index.js"
      },
      "require": {
        "types": "./dist-cjs/index.d.ts",  // Notice how typings for commonjs points to dist-cjs instead of dist
        "default": "./dist/index.cjs"
      }
    }
  },
  "type": "module",
  "files": [
    "dist",
    "dist-cjs"  // don't forget to include this otherwise the folder won't be published
  ],
}

@michaeldzjap
Copy link
Owner

However, now the commonjs typescript build is broken

Could you tell me what the error is you are getting? I created a simple sandbox of my own, testing out the CJS build for a typescript project and it seems to run without any problems now, unless I'm missing something?

@Cellule
Copy link
Author

Cellule commented Aug 11, 2024

Yeah I updated my sandbox https://codesandbox.io/p/devbox/confident-orla-xy9d9n

I get 2 errors

  1. cjs/typescript.ts(1,28): 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("rand-seed")' call instead.
  2. node_modules/rand-seed/dist/index.d.ts(1,28): 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.

first issue is because typescript thinks the commonjs file cjs/typescript.ts is trying to import a purely ESM package (hence my dist-cjs solution)
second issue is the missing .js in import Rand, { PRNG } from './Rand.js';

I can't access your devbox (make sure it's public in the "share" section)

@michaeldzjap
Copy link
Owner

You should be able to acces it now.

I think your issues might be because the "module" property in your tsconfig.json is set to "NodeNext". I've set it to "CommonJS" in my devbox example and that seems to work.

@Cellule
Copy link
Author

Cellule commented Aug 11, 2024

Yeah, but "NodeNext" is a valid option even in commonjs
I'm currently undergoing the migration to ESM on my main project and I'm doing small iterations toward ESM and transitioning to "NodeNext" was a step in that direction.
I mean at this point this package pretty much works in most scenarios, worst case I'll do a quick yarn patch on my side until I'm done with the ESM migration (which truth be told has been ongoing for like 2 years now 😅)

@michaeldzjap
Copy link
Owner

Yeah, but "NodeNext" is a valid option even in commonjs

Fair point. In fact, it seems the TypeScript documentation recommends it for modern projects. I'll have another go at it when I have time. I'd like to find a solution for this, but boy oh boy, this is not very fun stuff. I'm using Rollup for bundling / transpiling, so ideally I find a way to make this all work purely using that instead of having to manually create any "patching" files myself. Or maybe it's time to look for another tool for bundling that makes this all a little easier (if such a thing exists).

@michaeldzjap michaeldzjap reopened this Aug 11, 2024
@michaeldzjap michaeldzjap linked a pull request Aug 18, 2024 that will close this issue
@michaeldzjap michaeldzjap reopened this Aug 18, 2024
@michaeldzjap
Copy link
Owner

michaeldzjap commented Aug 18, 2024

OK, feel like I am another step closer. 3 of the 4 builds work now at least with v2.1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants