-
-
Notifications
You must be signed in to change notification settings - Fork 1k
test: extract cjs require tests to .spec.cts #3436
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.
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
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3436 +/- ##
=======================================
Coverage 99.97% 99.97%
=======================================
Files 2880 2880
Lines 220510 220510
Branches 952 952
=======================================
Hits 220457 220457
Misses 53 53 🚀 New features to boost your workflow:
|
88640f0
to
0fb1a6a
Compare
0fb1a6a
to
b6c8cb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we just move all require(*)-cjs calls into .spec.cts files. That way we can more safely use the require-nodejs keyword and ensure that it really works.
What do you mean by "ensure that it really works"? Why does it not work right now? 🤔
But not only this, when we will remove cjs support in v10, we could ensure this way at least for newer versions of nodejs, that it still works with their new require(esm) nodejs.org/api/modules.html#loading-ecmascript-modules-using-require
I don't see how moving the test in a separate file ensures this behavior. As far as I can tell this would require our code base to use explicit *.m(t|j)s
file extensions, which is not the case right now. Am I missing something? Could you elaborate on this?
Before this PR, we used faker/test/locale-imports.spec.ts Line 9 in 1e551c5
No, we wont need to ship |
36cdb4b
to
7a036ed
Compare
@xDivisionByZerox in the future (v10) we might need to run this And then we need to switch the required calls to depend on v10's esm bundled code - require('../dist/index.cjs')
+ require('../dist/index.js')
- require(`../dist/locale/${locale}.cjs`)
+ require(`../dist/locale/${locale}.js`) |
7a036ed
to
e5a5074
Compare
1f87d91
to
e1ad736
Compare
e1ad736
to
f852547
Compare
88e4bba
to
872d1dd
Compare
@xDivisionByZerox just an info, but #3540 (comment) fails with node 18 Do you still wont like this cts test file change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xDivisionByZerox just an info, but #3540 (comment) fails with node 18
But this PR here would also still run with node 18, because it actively still tests it with native cjs support and does not fake a require inside an esm fileDo you still wont like this cts test file change?
That was the argument (or proof of concept) I was waiting for 👍
I just go a great idea over night! 💡
What if we just move all
require(*)
-cjs calls into.spec.cts
files. That way we can more safely use therequire
-nodejs keyword and ensure that it really works.But not only this, when we will remove cjs support in v10, we could ensure this way at least for newer versions of nodejs, that it still works with their new
require(esm)
https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require