faker-js/faker

[9.0.0] Types are not properly emitted

notaphplover opened this issue · 16 comments

notaphplover commented

Pre-Checks

Describe the bug

@faker-js/faker@9.0.0 does not emit typescript types properly. Consider this tool as reference.

Consider grafana/pyroscope-nodejs#37 as reference.

Minimal reproduction code

Simply install the library in NodeJS >= 16 and try to import it in a cjs module.

tsc will complain with the following error:

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("@faker-js/faker")' call instead.

Additional Context

Having a look at the package.json:

{
  "exports": {
    ".": {
      "types": "./dist/types/index.d.ts",
      "import": "./dist/index.js",
      "require": "./dist/index.cjs",
      "default": "./dist/index.js"
    },
    "./locale/*": {
      "types": "./dist/types/locale/*.d.ts",
      "import": "./dist/locale/*.js",
      "require": "./dist/locale/*.cjs",
      "default": "./dist/locale/*.js"
    },
    "./package.json": "./package.json"
  }
}

This is wrong. Types emitted at ./dist/types/ are esm types, breaking cjs requires. Instead of doing this, types should be emitted in both cjs and esm builds so typescript can infer the right types. Consider grafana/pyroscope-nodejs#37 as reference

Environment Info

System:
    OS: Linux 6.8 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
    CPU: (8) x64 Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
    Memory: 24.45 GB / 31.24 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 20.9.0 - ~/.nvm/versions/node/v20.9.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v20.9.0/bin/yarn
    npm: 10.8.2 - ~/.nvm/versions/node/v20.9.0/bin/npm
    pnpm: 9.9.0 - ~/.nvm/versions/node/v20.9.0/bin/pnpm
  Browsers:
    Chrome: 127.0.6533.119

Which module system do you use?

  • CJS
  • ESM

Used Package Manager

pnpm

ST-DDT commented

@notaphplover I'm unable to reproduce this using our playgrounds.

https://github.com/faker-js/playground/tree/main/playgrounds/cjs or other *-cjs

Could you please provide a minimal viable reproduction?

  • 👍1
notaphplover commented

@notaphplover I'm unable to reproduce this using our playgrounds.

https://github.com/faker-js/playground/tree/main/playgrounds/cjs or other *-cjs

Could you please provide a minimal viable reproduction?

Hey @ST-DDT, I certainly can 😃, consider https://github.com/notaphplover/faker-js-faker-3087-repro as a reproduction repo.

Steps to reproduce the issue

  1. Clone the repo.
  2. Install depedencies.
  3. Run the build npm script.

Expected behavior

  • tsc is able to compile source code.

Current behavior

  • tsc crashes with the following error:
src/index.ts:1:23 - 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("@faker-js/faker")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '/home/bob/Documents/repos/faker-js-faker-3087-repro/package.json'.

1 import { faker } from "@faker-js/faker";
                        ~~~~~~~~~~~~~~~~~
notaphplover commented

Regarding https://github.com/faker-js/playground/tree/main/playgrounds, it seems you are not covering typescript NodeNext modules. If you want to test this in the future, I would suggest you to have a look at https://arethetypeswrong.github.io/, it's the simplest way to quickly check a package typescript types.

marklai1998 commented

I'm getting this under a NodeNext project

Error: node_modules/.pnpm/@faker-js+faker@9.0.0/node_modules/@faker-js/faker/dist/types/index.d.ts(1,548): 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.
Shinigami92 commented

As an immediate workaround you can use "moduleResolution": "Node10" (or "moduleResolution": "Bundler" if you can use a bundler like esbuild, vite, tsup, swc, you name it...).
We will analyze the problem, but we do not want to duplicate our types, because this would add additional 500kb to the already huge package size.
Also be prepared that we plan to remove support for CJS in Faker v10.

notaphplover commented

As an immediate workaround you can use "moduleResolution": "Node10" (or "moduleResolution": "Bundler" if you can use a bundler like esbuild, vite, tsup, swc, you name it...).

To be very honest, I find an easier inmediate workaround to simply not to upgrade to faker@9. faker@8 works fine.

We will analyze the problem, but we do not want to duplicate our types, because this would add additional 500kb to the already huge package size.

It's not about duplicating types, it's about providing valid types. cjs types are not the same as esm types in the same way cjs modules are not the same than esm modules. I don't think adding 500kb is such a concern, either faker is a dev dependency and this increase of space simply doesn't matter, or either is not and, in the cases in which this might matter, a tree shaking strategy in a bundler should deal with it easily.

Also be prepared that we plan to remove support for CJS in Faker v10.

I think this is a huge mistake. Most backend developers cannot afford the luxury of migrating to ESM modules given the current state of some libraries, like jest, whose esm support is experimental. I would love to migrate my source code to esm, but I simply cannot afford it in the backend. It's way easier to just get rid of faker. Of course, this is just my opinion.

  • 👍2
marklai1998 commented

I would Will stay on v8 for now
Duplicate type should be fine, as @notaphplover mentioned, most user would use it as dev deps so 500kb is not a concern for most of the ppl. And Faker v10 gonna drop cjs anyway, so there is no point to make broken types in V9

For dropping cjs I'm personally dont mind the change, we're heading toward ESM and in future Node version you can require ESM, old project can stay on older version if necessary, but its kinda off topic here, worth a separate discussion

(maybe I should raise a separate issue with NodeNext, this issue is primary for cjs build

  • 👍1
matthewmayer commented

I think it would make sense to open a seperate issue or discussion for dropping CJS support in future versions to avoid this issue running off topic. Then people who have concerns about that (myself included) can discuss there.

I think for this issue as it appears to be a regression we should at minimum add something in the 9.0 migration guide as a known issue.

  • 👍2
Shinigami92 commented

I think for this issue as it appears to be a regression we should at minimum add something in the 9.0 migration guide as a known issue.

The current problem is more that the Faker maintainers are running at limited capacity. Adding a known-issue to the guide takes potentially the same amount of effort from us maintainers as fixing / tackling the issue itself.

matthewmayer commented

I think for this issue as it appears to be a regression we should at minimum add something in the 9.0 migration guide as a known issue.

The current problem is more that the Faker maintainers are running at limited capacity. Adding a known-issue to the guide takes potentially the same amount of effort from us maintainers as fixing / tackling the issue itself.

If it's an easy fix then sure. If it might take longer adding a sentence to migration guide in the meantime seems like low hanging fruit.

Jym77 commented

Adding that the types also seem to not work under ESM (at least under certain configurations).

The import are written without extension, e.g. src/index.ts contains export { FakerError } from './errors/faker-error';. This is more or less copied as-is in dist/types in the package.

But under ESM + Node16 resolution (TS), relative imports must be the full file name, including extension. When updating from 8.4.1 to 9.0.0, I get:

node_modules/@faker-js/faker/dist/types/index.d.ts:41:42 - 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.

41 export { SimpleFaker, simpleFaker } from './simple-faker';

when I tsc build.

Somehow, I wasn't getting that on 8.4.1 which has the same imports. I assume it comes from the internal switch to ESM after 8.4.1 🤔

  • 👍1
  • 👀1
Shinigami92 commented

It's unbelievable how complex the JS ecosystem is. It's a mess. 🫠
I'm more and more longing for v10 of Faker where we can leave all this overhead of CJS behind and just move on with simple builds.
Until then I assume #3093 will fix it.

  • 👍1
notaphplover commented

It's unbelievable how complex the JS ecosystem is. It's a mess. 🫠 I'm more and more longing for v10 of Faker where we can leave all this overhead of CJS behind and just move on with simple builds.

As other users mentioned, we shouldn't run offtopic. If faker maintainers would like to discuss about it, I just started #3103.

  • 👍1
Jym77 commented

I confirm that the newer 9.0.0 9.0.1 is working in my setup 🎉
[edit: fix version number]

  • ❤️2
ST-DDT commented

I assume you are referring to v9.0.1 instead of v9.0.0

Jym77 commented

Yes 🙈 Misread the semver range for the lockfile… 🙃

  • 😄1