Skip to content

refactor!: experimental v10 #3513

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: test-cjs-require
Choose a base branch
from
Draft

refactor!: experimental v10 #3513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0e396f
refactor!: experimental v10
Shinigami92 May 12, 2025
cbc8d6c
Update package.json
Shinigami92 May 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
node_version: [18, 20, 22]
node_version: [20, 22, 24]
fail-fast: false
timeout-minutes: 10

Expand Down Expand Up @@ -51,7 +51,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
node_version: [20]
node_version: [22]
fail-fast: false
env:
LANG: zh_SG.UTF-8
Expand Down
7 changes: 2 additions & 5 deletions eslint.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,10 @@ import eslintPluginFileProgress from 'eslint-plugin-file-progress';
import eslintPluginJsdoc from 'eslint-plugin-jsdoc';
import eslintPluginPrettierRecommended from 'eslint-plugin-prettier/recommended';
import eslintPluginUnicorn from 'eslint-plugin-unicorn';
import { dirname, resolve } from 'node:path';
import { fileURLToPath } from 'node:url';
import { resolve } from 'node:path';
import tseslint from 'typescript-eslint';

const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
const gitignorePath = resolve(__dirname, '.gitignore');
const gitignorePath = resolve(import.meta.dirname, '.gitignore');

const config: ReturnType<typeof tseslint.config> = tseslint.config(
//#region global
Expand Down
16 changes: 4 additions & 12 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,20 @@
"type": "module",
"exports": {
".": {
"require": {
"types": "./dist/index.d.cts",
"default": "./dist/index.cjs"
},
Comment on lines -70 to -73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put this question in a file comment. That way we can resolve the discussion at the end and do not have trillions of comments to scroll through.

How does removing the CJS entries benefit the Faker project directly? All of the changes presented are infrastructural related. I fail to see why we would deny a good amount of our users the ability to continue using our library when there is no compelling reason to exclude them.

From what I can see, the only apparent benefit is a reduction in build size (and consequently download size) since we would no longer have to build both ESM and CJS versions. While reducing the package size is certainly a goal for Faker I want to support for environmental reasons, achieving that at the cost of backward compatibility does not seem like the best approach to me.

Instead, I'd like to revisit my solution from discussion #3103. By transitioning from a monolithic project structure to a package-based one, we could potentially reduce the download size to even less than half of its current size, since users would only install the modules they actually need. I recognize that this approach would be challenging to implement, but it would certainly be more inclusive than discarding parts of our community entirely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can see, the only apparent benefit is a reduction in build size (and consequently download size) since we would no longer have to build both ESM and CJS versions. While reducing the package size is certainly a goal for Faker [...]

This is literally exactly what the benefit is.

As far as I can tell, with the new require(esm) support by NodeJS, we do not have any downsides anymore, and NodeJS 18 is EOL as well already.

However, as I'm still open to refactor the project later to a mono-repo with separated packages, this would be a huge task and potentially also based on how we design the new package structure, a breaking change for the users.
And as cjs is an actively dying thing, we would focus power into something that would die sooner or later anyways. So why doing it in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which versions of Node support require(esm)? Has it been backported to any LTS versions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I posted a link in the PR's description. According to this, it looks like that it was backported.

image

Beside this, even our CI test using Node v20 is running green. And we have a test for checking a require from dist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tested this out with https://www.npmjs.com/package/node-fetch which went ESM-only in node-fetch@3

With this sample code which works in node-fetch@2

const fetch = require("node-fetch")
fetch("https://api.github.com/users/octocat")
    .then((res) => res.json())
    .then((data) => {
        console.log(data);
    });

If you update to node-fetch@3 using Node 20.16.0 for example you get

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/matt/fetchtest/node_modules/node-fetch/src/index.js from /Users/matt/fetchtest/index.js not supported.
Instead change the require of /Users/matt/fetchtest/node_modules/node-fetch/src/index.js in /Users/matt/fetchtest/index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/matt/fetchtest/index.js:1:15) {
  code: 'ERR_REQUIRE_ESM'
}

If you update to Node 20.19.0 the error now shows

TypeError: fetch is not a function
    at Object.<anonymous> (/Users/matt/fetchtest/index.js:2:1)
    at Module._compile (node:internal/modules/cjs/loader:1529:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)
    at Module.load (node:internal/modules/cjs/loader:1275:32)
    at Module._load (node:internal/modules/cjs/loader:1096:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:164:12)
    at node:internal/main/run_main_module:28:49

Updating the code to

const fetch = require("node-fetch").default
fetch("https://api.github.com/users/octocat")
    .then((res) => res.json())
    .then((data) => {
        console.log(data);
    });

works.

So basically we would need to ensure that CJS users both:

  • Upgrade to a minimum of Node 20.19.0 or 22.12.0 or 23.0.0
  • Modify their require() to add the .default

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrade to a minimum of Node 20.19.0 or 22.12.0 or 23.0.0

Correct, we can document this in the release notes / changelog, or if we have a migration guide for v10 in that.

Modify their require() to add the .default

Incorrect, faker does not have a default export. So this isn't even required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] According to this, it looks like that it was backported.

I missed that in the documentation. Under these circumstances, I'll take back my previous concerns.
A new concern that arises would be the stability level of 1.2. While no breaking changes are anticipates, they might still happen. That's just something I'm thinking about.

Correct, we can document this in the release notes / changelog, or if we have a migration guide for v10 in that.

I assume that we would also enforce these requirements by correctly adjusting the engines field in our package.json. The changes you listed on top where intended for enhancing the DX/UX further than the bare requirements, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we can document this in the release notes / changelog, or if we have a migration guide for v10 in that.

I assume that we would also enforce these requirements by correctly adjusting the engines field in our package.json. The changes you listed on top where intended for enhancing the DX/UX further than the bare requirements, right?

Oh yeah, you are correct about the engines field!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think if we do this for v10 then it will definitely make sense to increase the minimum required versions in the engines field to match the versions that support this new syntax for importing ESM modules from CJS projects.

I believe the "engines" field is only advisory though. So even if you specify minimum v20.19.0 you can still install the library if you are using say v20.9.0.

So we should make sure our upgrading guide is clear on what version has supported and any error messages you might get if you are using an older Node version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.npmjs.com/cli/v11/configuring-npm/package-json#engines

Unless the user has set the engine-strict config flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.

So it will already show a warning

"default": {
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
}
},
"./locale/*": {
"require": {
"types": "./dist/locale/*.d.cts",
"default": "./dist/locale/*.cjs"
},
"default": {
"types": "./dist/locale/*.d.ts",
"default": "./dist/locale/*.js"
}
},
"./package.json": "./package.json"
},
"main": "dist/index.cjs",
"main": "dist/index.js",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I could NOT remove this line, because otherwise the require (cjs) > simpleFaker > should not log anything on startup fails (successfully) in line 38

https://github.com/faker-js/faker/pull/3513/files#diff-1a17b46bc8c7a1286275d009e0a040f5ae9f21b6232d8f7f5945bbf1780a0b82R38

"module": "dist/index.js",
"types": "dist/index.d.ts",
"typesVersions": {
Expand Down Expand Up @@ -126,7 +118,7 @@
"eslint-plugin-prettier": "5.2.6",
"eslint-plugin-unicorn": "58.0.0",
"jiti": "2.4.2",
"npm-run-all2": "7.0.2",
"npm-run-all2": "8.0.1",
"prettier": "3.5.3",
"prettier-plugin-organize-imports": "4.1.0",
"prettier-plugin-packagejson": "2.5.10",
Expand All @@ -146,8 +138,8 @@
},
"packageManager": "[email protected]",
"engines": {
"node": ">=18.0.0",
"npm": ">=9.0.0"
"node": ">=20.19.0",
"npm": ">=10.0.0"
},
"pnpm": {
"ignoredBuiltDependencies": [
Expand Down
Loading
Loading