26.0.6 node-module-collector RangeError: Maximum call stack size exceeded

[absidue]

I just attempted to update from electron-builder 25.1.8 to 26.0.6 and received the RangeError: Maximum call stack size exceeded error from the node-module-collector (full error below). The irony of that is that we don't even need electron-builder to create a list of node_modules to include, as we explicitly exclude them from being packaged with !node_modules/**/* in the files array, as we bundle the JavaScript code before calling electron-builder.

So this issue is a bug report but also an ask for electron-builder to check the files array before collecting the node_modules, so that it can avoid doing work that isn't necessary. If checking the files array isn't possible a boolean config option to skip the node_modules collection would also work.

RangeError: Maximum call stack size exceeded
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:54:21)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)
    at flatten (...\node_modules\app-builder-lib\src\node-module-collector\nodeModulesCollector.ts:69:9)

[beyondkmp]

Can you provide a minimum reproducible repo or instructions on how to repro your issue?

[absidue]

I'll work on trying to create a minimum reproduction.

Once I do provide a reproduction I ask you to leave this issue open until both issues are solved, only fixing the call stack problem only fixes one of the problems and it's the smaller of the two problems at that. The bigger issue is running the node_modules collector when it is not necessary, if that hadn't happened then the call stack issue wouldn't have occurred either.

[beyondkmp]

The files field uses glob rules, which makes it difficult to precisely filter out all node_modules. Perhaps a configuration option could be provided to exclude all node_modules from the packaging process. This way, it would be explicitly clear that the node module collector should not run.

I'm a bit curious, if none of the node_modules are packaged, will the application still run properly? Are you bundling all the code and dependencies into the dist directory?

[absidue]

Yes, it runs and better that it did before. We use webpack to bundle the main and renderer JavaScript files, we've been doing it like this for a few years now. Not shipping the node_modules means that the app is a lot smaller and also launches faster as it doesn't have to do all the imports at runtime.

Bundling the code is also recommended in Electron's best practices performance guide: https://www.electronjs.org/docs/latest/tutorial/performance#7-bundle-your-code

If there is a dedicated config option to disable the node modules collection that would be great too. Personally I would argue the default should be that node_modules collection is disabled, to encourage best practices and avoid people shipping bloated apps, but I can understand that for historical reasons it would be difficult to change the default.

[beyondkmp]

Are your node_modules empty when you are packaging? You can add a DEBUG=electron-builder to output more logs and take a closer look.

[beyondkmp]

Do you not have any native Node.js module dependencies? If users have native modules, then it will be necessary to package the node_modules.

[absidue]

As mentioned above, we have no reason to package the node_modules directory in our app, it is plain JavaScript and all dependencies are bundled and optimised in with webpack before we call electron-builder. The node_modules directory exists on the file system at the time of running electron-builder but because we have !node_modules/**/* in the files array the node_modules directory is not included in the app.asar file (which is exactly what we want). Our app has been running fine for years now without shipping the node_modules directory to the users.

I'll try debugging the call stack problem and creating a minimal reproduction later.

[mmaietta]

The node_modules directory exists on the file system at the time of running electron-builder but because we have !node_modules/**/* in the files array the node_modules directory is not included in the app.asar file (which is exactly what we want)

I agree with this, we shouldn't collect node_modules if they're already supposed to be excluded. There's gotta be a way internally we can determine this, as I don't recall this being an issue with the previous node module collector via app-builder-bin

I'm a bit curious, if none of the node_modules are packaged, will the application still run properly? Are you bundling all the code and dependencies into the dist directory?

I actually had this exact same setup in a previous project as everything bundled into a single file, then compiled into bytecode, and it worked extremely well (especially in terms of adding an additional layer of security)

@absidue, when you can provide a minimum repro repo, I can get started on resolving this asap. @beyondkmp maybe we can split up the work so that you tackle the callstack issue while I work on the other issue of unnecessarily collecting node modules (without adding an additional config param)

[absidue]

Looks like having @seald-io/nedb in the dependencies list in the package.json file causes this to happen. I was planning to create a unit test like you did in the other issue, but as it is taking me a while to understand the test system and how I could run just one test, I thought I would mention the package here already.

Yes, I am aware that an easy solution would be to move all our runtime dependencies to the devDependecies array as we bundle them anyway. As there is no field for "dependecies that are used by the runtime code and bundled inside of it" and adding comments to the package.json is only possible in non-standard package-manager specific way, sticking them in the dependencies list is the best option for us to separate between runtime dependencies and those that are only needed for development.

[mmaietta]

Thanks for isolating the package! I'll get on this.

Yes, I am aware that an easy solution would be to move all our runtime dependencies to the devDependecies array as we bundle them anyway.

Yeah, that's a valid workaround, but I don't think the onus should be on the end-user/dev. This should be something that is handled correctly within electron-builder and behave like it did before. The node-module-collector was an enhancement/migration to fix support for newer package managers, especially with hoisted modules; it was supposed to be unnoticed by end-users.

[mmaietta]

@absidue I'm unfortunately unable to reproduce in both unit tests and in my test project with @seald-io/nedb added to package.json deps. I think I may actually need a minimum repro repo to resolve the RangeError issue or even the list of deps copied from your package.json would help

I'll continue looking into the "if node_modules excluded, don't collect node_modules" request though

[vassbo]

I had the same issue upgrading from 25.1.8 to 26.0.6.
Tried electron-builder 26.0.1 and that worked fine!

I don't have the @seald-io/nedb package.

It happens right after the electron build is downloaded:

[beyondkmp]

@vassbo Thanks. I can reproduce it by unit test now.

test.ifAll("yarn max stack", () =>
  assertPack(
    "test-app-hoisted",
    {
      targets: Platform.WINDOWS.createTarget(DIR_TARGET, Arch.x64),
    },
    {
      isInstallDepsBefore: true,
      projectDirCreated: async (projectDir, tmpDir) => {
        await outputFile(path.join(projectDir, "yarn.lock"), "")
        await modifyPackageJson(projectDir, data => {
          data.dependencies = {
            "npm-run-all": "^4.1.5",
          }
        })
      },
      packed: context => verifyAsarFileTree(context.getResources(Platform.WINDOWS)),
    }
  )
)

[mmaietta]

@absidue @vassbo can you please try this patch-package first and verify that it works from your sides?
app-builder-lib+26.0.6.patch

diff --git a/node_modules/app-builder-lib/out/node-module-collector/nodeModulesCollector.js b/node_modules/app-builder-lib/out/node-module-collector/nodeModulesCollector.js
index ee047e0..2ea7b8e 100644
--- a/node_modules/app-builder-lib/out/node-module-collector/nodeModulesCollector.js
+++ b/node_modules/app-builder-lib/out/node-module-collector/nodeModulesCollector.js
@@ -62,6 +62,9 @@ class NodeModulesCollector {
                     result[parentKey] = { dependencies: [] };
                 }
                 result[parentKey].dependencies.push(newKey);
+                if (node.skipCircularDeps) {
+                    continue;
+                }
                 flatten(value, newKey);
             }
         };
diff --git a/node_modules/app-builder-lib/out/node-module-collector/npmNodeModulesCollector.js b/node_modules/app-builder-lib/out/node-module-collector/npmNodeModulesCollector.js
index 825f638..81438c5 100644
--- a/node_modules/app-builder-lib/out/node-module-collector/npmNodeModulesCollector.js
+++ b/node_modules/app-builder-lib/out/node-module-collector/npmNodeModulesCollector.js
@@ -16,8 +16,10 @@ class NpmNodeModulesCollector extends nodeModulesCollector_1.NodeModulesCollecto
         var _a;
         const dependencies = tree.dependencies || {};
         const _dependencies = tree._dependencies || {};
-        if (dependencies && Object.keys(dependencies).length === 0) {
+        if (Object.keys(_dependencies).length > 0 && Object.keys(dependencies).length === 0) {
             tree.dependencies = ((_a = this.allDependencies.get(`${tree.name}@${tree.version}`)) === null || _a === void 0 ? void 0 : _a.dependencies) || {};
+            tree.skipCircularDeps = true;
+            return;
         }
         for (const [key, value] of Object.entries(dependencies)) {
             if (!_dependencies[key] || Object.keys(value).length === 0) {
diff --git a/node_modules/app-builder-lib/out/node-module-collector/yarnNodeModulesCollector.js b/node_modules/app-builder-lib/out/node-module-collector/yarnNodeModulesCollector.js
index 6e512f9..e531124 100644
--- a/node_modules/app-builder-lib/out/node-module-collector/yarnNodeModulesCollector.js
+++ b/node_modules/app-builder-lib/out/node-module-collector/yarnNodeModulesCollector.js
@@ -16,8 +16,10 @@ class YarnNodeModulesCollector extends nodeModulesCollector_1.NodeModulesCollect
         var _a;
         const dependencies = tree.dependencies || {};
         const _dependencies = tree._dependencies || {};
-        if (dependencies && Object.keys(dependencies).length === 0) {
+        if (Object.keys(_dependencies).length > 0 && Object.keys(dependencies).length === 0) {
             tree.dependencies = ((_a = this.allDependencies.get(`${tree.name}@${tree.version}`)) === null || _a === void 0 ? void 0 : _a.dependencies) || {};
+            tree.skipCircularDeps = true;
+            return;
         }
         for (const [key, value] of Object.entries(dependencies)) {
             if (!_dependencies[key] || Object.keys(value).length === 0) {