refactor!: experimental v10 #3513
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## test-cjs-require #3513 +/- ##
=================================================
Coverage 99.97% 99.97%
=================================================
Files 2880 2880
Lines 220746 220746
Branches 950 951 +1
=================================================
Hits 220691 220691
Misses 55 55 🚀 New features to boost your workflow:
|
| "default": { | ||
| "types": "./dist/locale/*.d.ts", | ||
| "default": "./dist/locale/*.js" | ||
| } | ||
| }, | ||
| "./package.json": "./package.json" | ||
| }, | ||
| "main": "dist/index.cjs", | ||
| "main": "dist/index.js", |
There was a problem hiding this comment.
note: I could NOT remove this line, because otherwise the require (cjs) > simpleFaker > should not log anything on startup fails (successfully) in line 38
7b6b845 to
d0e396f
Compare
| "require": { | ||
| "types": "./dist/index.d.cts", | ||
| "default": "./dist/index.cjs" | ||
| }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Which versions of Node support require(esm)? Has it been backported to any LTS versions?
There was a problem hiding this comment.
I posted a link in the PR's description. According to this, it looks like that it was backported.
Beside this, even our CI test using Node v20 is running green. And we have a test for checking a require from dist.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
[...] 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?
There was a problem hiding this comment.
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
enginesfield in ourpackage.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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
package.json
Outdated
| @@ -146,8 +138,8 @@ | |||
| }, | |||
| "packageManager": "pnpm@10.8.0", | |||
| "engines": { | |||
| "node": ">=18.0.0", | |||
| "npm": ">=9.0.0" | |||
| "node": ">=20.11.0", | |||
There was a problem hiding this comment.
As discussed let's update this to the subset versions which support importing ESM modules from CJS.
There was a problem hiding this comment.
oh good catch 👍
| "node": ">=20.11.0", | |
| "node": ">=20.19.0", |
There was a problem hiding this comment.
"node": ">=20.19.0 <21 || >=22.13.0 <23 || >=23.5.0"
There was a problem hiding this comment.
"node": ">=20.19.0 <21 || >=22.13.0 <23 || >=23.5.0"
should we maybe do "node": ">=20.19.0 <21 || >=22.12.0 <23 || >=23.0.0" instead?
because I don't care if someone gets a warning of using an experimental flag or not, but it works anyway.
And would it mean we do not support Node 21? I personally don't care because 21 would be not officially supported anyway by the Node Team.
There was a problem hiding this comment.
I don't think we need to worry about old odd versions. If you choose to use an odd version typically it's for a very specific use case and you know what you're doing.
This is an experimental PR based on top of #3436
it updates the lowest node support to v20 and generates only esm and no cjs
however, the cjs support is still given due to node's
require(esm)https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require
I will split this PR later into esm-only and node v20 PRs
this PR is more to check if CI and everything else is working as expected
https://node.green/#ES2023