-
Notifications
You must be signed in to change notification settings - Fork 373
No longer accept .js config files #3519
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
e187fb0 to
82926b0
Compare
rpl
left a comment
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.
note: I second the removal of the other custom error message and related logic, in the end that would provide the developer with the actual error message that got raised by the dynamic import, which feels more valuable than the custom error messages we use to be logging if one of the known issue were being hit.
The inline review comments below includes a few additional thoughts related to the details being show to the user when a config file error is hit, but none of them feels to be blocking to me (a couple are just nits and one is actually an enhancement on top of these changes) and so I'm also approving this PR as is.
Feel free to bring the sign-off along if you decide to tweak the patch to handle some of those nits and/or enhancement.
Fixes #3429, maybe
about the maybe part from this PR comment, there may be one more hit by the config file test on nodejs 24 and I was also hitting a more general ESM mocking issue on both nodejs 22 and nodejs 24 while running the unit tests locally (this one not strictly related to the config file thought).
The following diff fixed both those failures for me locally (locally I tried on nodejs 22.17.0 and nodejs 24.8.0):
diff --git a/scripts/lib/mocha.js b/scripts/lib/mocha.js
index 79ea5bf..25c6931 100644
--- a/scripts/lib/mocha.js
+++ b/scripts/lib/mocha.js
@@ -20,10 +20,6 @@ const runMocha = (args, execMochaOptions = {}, coverageEnabled) => {
shell.echo(`\nSetting mocha timeout from env var: ${MOCHA_TIMEOUT}\n`);
}
- // Pass testdouble node loader to support ESM module mocking and
- // transpiling on the fly the tests modules.
- binArgs.push('-n="loader=testdouble"');
-
const res = spawnSync(binPath, binArgs, {
...execMochaOptions,
env: {
diff --git a/src/config.js b/src/config.js
index d3dd22f..418c372 100644
--- a/src/config.js
+++ b/src/config.js
@@ -145,15 +145,23 @@ export async function loadJSConfigFile(filePath) {
// ES modules may expose both a default and named exports and so
// we merge the named exports on top of what may have been set in
// the default export.
+ if (filePath.endsWith('.cjs')) {
+ // Remove the additional 'module.exports' named exports that
+ // nodejs >= 24 is returning from the dynamic import call (in
+ // addition to being also set on the default property as in
+ // nodejs < 24).
+ delete esmConfigMod['module.exports'];
+ }
configObject = { ...configDefault, ...esmConfigMod };
} else {
configObject = { ...configModule };
}
} catch (error) {I haven't tried to see if the circleci job would also be hitting them, but it is reasonable to believe the CI jobs would also be hitting the same, but in the meantime I thought to mention both here in case you would hit them next.
(I haven't tried to run the tests on windows and so I'm not sure if there are any other hit only hit there).
82926b0 to
4b2727d
Compare
4b2727d to
71282f6
Compare
Fixes #3429