notaphplover commented 1 week ago

Pre-Checks

[X] Follow our Code of Conduct.
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Faker issue and not related to a combination with another package.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.
[X] I am willing to provide a PR.

Describe the bug

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

Consider https://github.com/grafana/pyroscope-nodejs/issues/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 https://github.com/grafana/pyroscope-nodejs/issues/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?

[X] CJS
[ ] ESM

Used Package Manager

pnpm

ST-DDT commented 1 week ago

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

notaphplover commented 1 week ago

@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 :smiley:, consider https://github.com/notaphplover/faker-js-faker-3087-repro as a reproduction repo.

Steps to reproduce the issue

Clone the repo.
Install depedencies.
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 1 week ago

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 1 week ago

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 1 week ago

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 1 week ago

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.

marklai1998 commented 1 week ago

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

matthewmayer commented 1 week ago

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.

Shinigami92 commented 1 week ago

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 1 week ago

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 1 week ago

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 🤔

Shinigami92 commented 1 week ago

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.

3093

notaphplover commented 1 week ago

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 https://github.com/faker-js/faker/discussions/3103.

Jym77 commented 9 hours ago

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

ST-DDT commented 8 hours ago

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

Jym77 commented 7 hours ago

Yes 🙈 Misread the semver range for the lockfile… 🙃

faker-js / faker

[9.0.0] Types are not properly emitted #3087

Pre-Checks

Describe the bug

Minimal reproduction code

Additional Context

Environment Info

Which module system do you use?

Used Package Manager

Steps to reproduce the issue

Expected behavior

Current behavior

3093