Repository: electron/electron
Pull request: fix: do not load source for `electron` module in ESM loader synchronous flow

#### Description of Change

Mirrors a modification in the ESM patch that avoids loading source for the `electron` module to the same (synchronous) code used for CJS interoperability. This code path can only be triggered in packaged applications.

---

It was an interesting journey figuring out the root cause for this one.

There exists two flows in the ESM loader: asynchronous (ESM) and synchronous (CJS). The second flow enables CJS modules to load ESM modules and vice versa. When a CJS module loads an ESM module, its `import`s are loaded synchronously for compatibility.

We [patch](https://github.com/electron/electron/blob/c2ab63f6d7091f9412362136d3ed66a563d33082/patches/node/fix_do_not_resolve_electron_entrypoints.patch#L11-L20) the ESM loader to avoid loading source code for the `electron` module (because there is none). The patch only applies to the asynchronous flow. The patch also needs to be applied to the synchronous flow, which only runs on the `electron` module in packaged applications.

In an *unpackaged* application, [`default_app`](https://github.com/electron/electron/tree/main/default_app) is the entrypoint. At the beginning of `default_app`, [the `electron` module is loaded](https://github.com/electron/electron/blob/c2ab63f6d7091f9412362136d3ed66a563d33082/default_app/main.ts#L1) via `import` as an ESM module. Loading the `electron` module this way uses the (patched) asynchronous flow. As a side effect, the module instance gets cached.

In a *packaged* application, the app's main script is the entrypoint. `default_app` is not present and does not run. As a consequence, the module cache is relatively fresh.

The domino effect starts when (1) a _CJS module_ loads via `require` (2) an _ESM module_ that loads via `import` (3) the `electron` module (4) for the _first time_.[^1][^2] The ESM loader's synchronous flow does not take the usual cache hit branch and instead attempts to load the module. Since the synchronous flow is not patched to avoid loading the `electron` module's source code, an error ends up getting thrown.[^3] 💥

[^1]: (1) The CJS module is necessary to trigger the synchronous flow; (2) The ESM module is necessary to trigger the ESM loader; (3) The `electron` module is special; (4) It must be the first time or else the module will be cached.
[^2]: The `electron/js2c/browser_init` module is loaded at startup, but it is a special case that uses the built-in code cache, completely circumvents most of the module loading code, and is distinct from the `electron` module.
[^3]: Somewhat interestingly, the thrown error is not from the file system. Instead, it's from a [validation check](https://github.com/nodejs/node/blob/b673c697a75f440b9a8c60a28737acc9b01cebe7/lib/internal/modules/esm/load.js#L71) on the resolved module path: `electron:` is not a protocol it knows how to load source for.

---

I also took the opportunity to move some code between patches, but honestly I don't know what goes where, so lmk if there's a better home for these changes. As for testing, I couldn't find a simple way to get our test runner to skip the `default_app` entrypoint, so I'm pushing the fix up by itself. If you accept "it works on my machine" though, then rest assured I manually verified this fix.

Fixes #46614

#### Checklist

- [x] PR description included and stakeholders cc'd
- [x] `npm test` passes
- [ ] tests are [changed or added](https://github.com/electron/electron/blob/main/docs/development/testing.md)
- [ ] relevant API documentation, tutorials, and examples are updated and follow the [documentation style guide](https://github.com/electron/electron/blob/main/docs/development/style-guide.md)
- [x] [PR release notes](https://github.com/electron/clerk/blob/main/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/main/README.md#examples).

#### Release Notes

Notes: Fixed an error when importing `electron` for the first time from an ESM module loaded by a CJS module in a packaged app.


Changed files:
filename: patches/node/fix_do_not_resolve_electron_entrypoints.patch
patch: @@ -5,19 +5,6 @@ Subject: fix: do not resolve electron entrypoints
 
 This wastes fs cycles and can result in strange behavior if this path actually exists on disk
 
-diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
-index c9d4a3536d0f60375ae623b48ca2fa7095c88d42..d818320fbbc430d06a0c2852e4723981d6e1a844 100644
---- a/lib/internal/modules/esm/load.js
-+++ b/lib/internal/modules/esm/load.js
-@@ -109,7 +109,7 @@ async function defaultLoad(url, context = kEmptyObject) {
-     source = null;
-     format ??= 'builtin';
-   } else if (format !== 'commonjs' || defaultType === 'module') {
--    if (source == null) {
-+    if (format !== 'electron' && source == null) {
-       ({ responseURL, source } = await getSource(urlInstance, context));
-       context = { __proto__: context, source };
-     }
 diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
 index 49aacb6262502ced54e817f99dd596db85b9659c..f9f065bb743275e9b2ce71375e6a9f06e00c0f36 100644
 --- a/lib/internal/modules/esm/translators.js

filename: patches/node/fix_expose_the_built-in_electron_module_via_the_esm_loader.patch
patch: @@ -18,9 +18,18 @@ index 9519f947b8dfdc69808839948c9cb8434a0acf0e..23ce72d479f638c33edffcea7c35f5da
  
  /**
 diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
-index 5ba13096b98047ff33e4d44167c2a069ccc5e69d..a00b5979e3b5deb4ba315b4635c7e5d2801c376e 100644
+index 5ba13096b98047ff33e4d44167c2a069ccc5e69d..09a332c0999086b30fd952d9456f788925240e27 100644
 --- a/lib/internal/modules/esm/load.js
 +++ b/lib/internal/modules/esm/load.js
+@@ -106,7 +106,7 @@ async function defaultLoad(url, context = kEmptyObject) {
+ 
+   throwIfUnsupportedURLScheme(urlInstance);
+ 
+-  if (urlInstance.protocol === 'node:') {
++  if (urlInstance.protocol === 'node:' || format === 'electron') {
+     source = null;
+     format ??= 'builtin';
+   } else if (format !== 'commonjs' || defaultType === 'module') {
 @@ -119,7 +119,7 @@ async function defaultLoad(url, context = kEmptyObject) {
        // Now that we have the source for the module, run `defaultGetFormat` to detect its format.
        format = await defaultGetFormat(urlInstance, context);
@@ -30,6 +39,15 @@ index 5ba13096b98047ff33e4d44167c2a069ccc5e69d..a00b5979e3b5deb4ba315b4635c7e5d2
          // For backward compatibility reasons, we need to discard the source in
          // order for the CJS loader to re-fetch it.
          source = null;
+@@ -167,7 +167,7 @@ function defaultLoadSync(url, context = kEmptyObject) {
+ 
+   throwIfUnsupportedURLScheme(urlInstance, false);
+ 
+-  if (urlInstance.protocol === 'node:') {
++  if (urlInstance.protocol === 'node:' || format === 'electron') {
+     source = null;
+   } else if (source == null) {
+     ({ responseURL, source } = getSourceSync(urlInstance, context));
 @@ -200,12 +200,13 @@ function throwIfUnsupportedURLScheme(parsed) {
      protocol !== 'file:' &&
      protocol !== 'data:' &&
@@ -45,6 +63,19 @@ index 5ba13096b98047ff33e4d44167c2a069ccc5e69d..a00b5979e3b5deb4ba315b4635c7e5d2
      throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(parsed, schemes);
    }
  }
+diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
+index fdecd0c926c953d33117d3d05366261ccf034a21..2e1ddd821ea6254fe00c52292620c3a58a62c046 100644
+--- a/lib/internal/modules/esm/loader.js
++++ b/lib/internal/modules/esm/loader.js
+@@ -426,7 +426,7 @@ class ModuleLoader {
+     }
+ 
+     const cjsModule = wrap[imported_cjs_symbol];
+-    if (cjsModule) {
++    if (cjsModule && finalFormat !== 'electron') {
+       assert(finalFormat === 'commonjs-sync');
+       // Check if the ESM initiating import CJS is being required by the same CJS module.
+       if (cjsModule?.[kIsExecuting]) {
 diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
 index bfd9bd3d127404de1cbb6f30c43ab0342590759d..9e7d8ef0adef3b68a3ec186e4b218f591aa69266 100644
 --- a/lib/internal/modules/esm/resolve.js

filename: patches/node/fix_lazyload_fs_in_esm_loaders_to_apply_asar_patches.patch
patch: @@ -6,7 +6,7 @@ Subject: fix: lazyload fs in esm loaders to apply asar patches
 Changes { foo } from fs to just "fs.foo" so that our patching of fs is applied to esm loaders
 
 diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
-index a00b5979e3b5deb4ba315b4635c7e5d2801c376e..c9d4a3536d0f60375ae623b48ca2fa7095c88d42 100644
+index 09a332c0999086b30fd952d9456f788925240e27..910ac85cdc86e7fb3f850f7edf0b95ea09d464dc 100644
 --- a/lib/internal/modules/esm/load.js
 +++ b/lib/internal/modules/esm/load.js
 @@ -10,7 +10,7 @@ const {