michaeldzjap / rand-seed

A small seedable random number generator library written in TypeScript
MIT License
27 stars 6 forks source link

Incorrect ESM build #10

Open Cellule opened 3 months ago

Cellule commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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

Cellule commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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


```json
// dist-cjs/package.json
{ "type": "commonjs" }
// ^ This overrides the default type of the package for the index.d.ts file in dist-cjs
michaeldzjap commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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