Skip to content
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

[Bug]: Regression: #35930 broke CJS via ESM loading when exporting null/undefined with ELECTRON_RUN_AS_NODE #36970

Closed
3 tasks done
Prinzhorn opened this issue Jan 20, 2023 · 3 comments · Fixed by #37009
Closed
3 tasks done

Comments

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Jan 20, 2023

Preflight Checklist

Electron Version

22.0.3

What operating system are you using?

Ubuntu

Operating System Version

Ubuntu 22.10

What arch are you using?

x64

Last Known Working Electron version

20.3.1 / 21.1.0

Expected Behavior

  1. npm i electron@20.3.1 ava
  2. ELECTRON_RUN_AS_NODE=true ./node_modules/.bin/electron ./node_modules/.bin/ava
  3. Works (can't find any tests obviously)

I was successfully running my esm tests via ELECTRON_RUN_AS_NODE until now

Actual Behavior

  1. npm i electron@20.3.2 ava
  2. ELECTRON_RUN_AS_NODE=true ./node_modules/.bin/electron ./node_modules/.bin/ava
  3. Crashes
node:internal/modules/esm/translators:178
      if (!ObjectPrototypeHasOwnProperty(exports, exportName) ||
           ^

TypeError: Cannot convert undefined or null to object
    at hasOwnProperty (<anonymous>)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:178:12)
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:533:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Testcase Gist URL

No response

Additional Information

I was able to track this down and repro without ava as well (avajs/ava#3155). It appears you can no longer export null because it crashes the esm loader patch.

index.mjs

import m from './module.cjs';

module.cjs

module.exports = null;

ELECTRON_RUN_AS_NODE=true ./node_modules/.bin/electron index.mjs

It broke between 20.3.1 and 20.3.2 because of #35930 . If you're curious, in ava this comes from this dep https://github.com/jamiebuilds/ci-parallel-vars/blob/994940e4882bdddf046a5c076b49d700449e4b38/index.js#L41-L56

Weirdly, this is like the 4th time I've tried this.

@MarshallOfSound fifth time is the charm

@Prinzhorn Prinzhorn changed the title [Bug]: Regression: #35930 broke CJS via ESM loading when exporting anything that is not an object [Bug]: Regression: #35930 broke CJS via ESM loading when exporting null Jan 20, 2023
@Prinzhorn Prinzhorn changed the title [Bug]: Regression: #35930 broke CJS via ESM loading when exporting null [Bug]: Regression: #35930 broke CJS via ESM loading when exporting null/undefined Jan 20, 2023
@Prinzhorn
Copy link
Contributor Author

Prinzhorn commented Jan 20, 2023

I think this is the culprit, because it assumes you can only export objects when in reality you can export anything.

Object.keys(module.exports)

@Prinzhorn Prinzhorn changed the title [Bug]: Regression: #35930 broke CJS via ESM loading when exporting null/undefined [Bug]: Regression: #35930 broke CJS via ESM loading when exporting null/undefined with ELECTRON_RUN_AS_NODE Jan 20, 2023
@Abhijrathod

This comment was marked as abuse.

@Prinzhorn
Copy link
Contributor Author

Prinzhorn commented Jan 22, 2023

Thanks for your reply.

  1. I already quoted fix: expose the built-in electron module via the ESM loader #35930 (even in the title) and mentioned the author of the PR
  2. It's not a module I control (as mentioned it comes from ci-parallel-vars which ava uses), hence I cannot not export null (which is perfectly valid code)
  3. Yes, I downgraded to 21.1.0 which is the most recent version without the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants