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

Nested node_modules disappears after building #3185

Closed
bradydowling opened this issue Jul 28, 2018 · 55 comments · Fixed by #5911
Closed

Nested node_modules disappears after building #3185

bradydowling opened this issue Jul 28, 2018 · 55 comments · Fixed by #5911

Comments

@bradydowling
Copy link

  • Version: 20.26.0
  • Target: macOS (.dmg)

Similar to #1539, I'm working with an app where I've cloned another git repo int and I have several node_modules folders. When I build, the node_modules folder in the nested git repo disappears. This happens whether asar is enabled or disabled. It also happens regardless of whether or not the inner git repo is officially designated as a git submodule or if I've just manually cloned it in.

Here's my folder structure:

main.js (starts app and forks process from the express app's index.js)
node_modules
package.json
cloned-express-app
   |
    --node_modules
      index.js
      package.json

Here's a Stack Overflow issue I just created that may explain it better (I didn't want to copy/paste but let me know if that's preferred) and here's the repo I'm working with.

It's also worth noting that I've tried building this for Windows and my friend said it works fine on there. I haven't tested that recently though so I can't give much info on that immediately. I also haven't inspected the Resources contents of the packaged app (or whatever the .exe counterpart would be) so I can't provide info on that but I do know that when I look in the mac app's Resources directory there is no node_modules folder within the nested git repo.

@fpsqdb
Copy link

fpsqdb commented Jul 30, 2018

Same issue here for me.

  • version: 20.26.0

  • target: windows

 "files": [
            "!**/**.map",
            "!**/**.ts",
            "!node_modules",
            "package.json",
            "node_modules/semver/**",
            "node_modules/fs-extra/**",
            "node_modules/graceful-fs/**",
            "node_modules/jsonfile/**",
            "node_modules/universalify/**"
        ],

When I build, there is no node_modules folder in the package. That's not my expected, it should have those folders and files in the package.

node_modules/semver,
node_modules/fs-extra,
node_modules/graceful-fs,
node_modules/jsonfile,
node_modules/universalify

But if I downgrade to 20.15.0, it works fine.

@bradydowling
Copy link
Author

Interesting, I'll need to give this a try, thanks for the note.

@bradydowling
Copy link
Author

bradydowling commented Aug 1, 2018

It did indeed package node_modules with this old version (20.15.0) on macOS as well, thank you @fpsqdb.

@hn3000
Copy link

hn3000 commented Oct 2, 2018

In version 20.16 the way node_modules includes work was changed: explicit includes are now ignored, instead all production dependencies are used. Unfortunately, excludes are still allowed to affect the node_modules folders and that means your !node_modules takes effect.

Looking at your example above:

    "files": [
        ...,
        "!node_modules",
        ...,
        "node_modules/semver/**",
        "node_modules/fs-extra/**",
        "node_modules/graceful-fs/**",
        "node_modules/jsonfile/**",
        "node_modules/universalify/**",
        ...
    ],

The first line will prevent inclusion of anything in the node_modules folder, the other lines are ignored since version 20.16, resulting in an app.asar that does not include any node_modules.

Remove the exclusion for node_modules and newer versions than 20.15 should work fine. In fact, you should be able to remove all lines mentioning the node_modules folder.

(Edit: tried to explain in more detail.)

@fpsqdb
Copy link

fpsqdb commented Oct 3, 2018

@hn3000 There's a condition that when use webpack to bundle packages, so you don't need all the dependencies. And if there are some packages exclude from webpack(such as set externals in webpack config) we should add them individually, that's my situations.

@hn3000
Copy link

hn3000 commented Oct 3, 2018

Then maybe a second package.json can help? https://www.electron.build/tutorials/two-package-structure

@TanninOne
Copy link

And a change like this was made in a minor update (20.15 -> 20.16)? Intentionally?

@TanninOne
Copy link

TanninOne commented May 2, 2019

Am I missing something here? How is this bug still not addressed?
Afaict all releases post 20.15.0 are unusable and should be considered extremely dangerous, how is this not top priority?
@develar ? I don't want to be annoying, but I've not been able to produce a working build with any electron-builder after 20.15.0 and I'm starting to panic as you don't even seem to acknowledge there is a problem here.
And no, hn3000s explanation does not fully explain the problem, I get this error even entirely without any files excludes.

My project depends on semver@^5.6.0
A dependency I include (semvish) depends on semver@^4.6.3
electron-builder packages semver@^4.6.3 twice, in semvish/node_modules and the top-level node_modules, causing bugs in my main application, breaking electron-updater among other things.
And I'll just assume this is one problem of many caused by this.

@grantcv1
Copy link

grantcv1 commented May 4, 2019

My application brings together 9 independent submodules (which have a life of their own in in other apps) and each has their own node_module. I've been using a much earlier version of electron-builder (19.55.3) for over a year without any issues -- all the node_modules were copied into my installer. After upgrading to the latest version, I've lost 3 days of development trying to understand this issue. It really need to be address. Seconding what TanninOne said, without fixing this issue, electron-builder is unusable.

@theproductiveprogrammer

Is there any update regarding this issue? Or any workaround? I've spent 2 days fiddling with various setting and nothing is working. I can't proceed to release an installer at this rate. Anyone have any suggestions?

@bradydowling
Copy link
Author

bradydowling commented Jun 11, 2019 via email

@theproductiveprogrammer
Copy link

theproductiveprogrammer commented Jun 12, 2019

I tried that but when I rolled back to 20.15.0 I got a new error:

[xmldom error]  invalid doc source
@#[line:0,col:undefined]
TypeError: Cannot read property 'documentElement' of undefined
    at parse
(ai-ico/package/elife-node/node_modules/plist/lib/parse.js:68:9)
    at
ai-ico/package/elife-node/node_modules/electron-builder-lib/src/electron/electronMac.ts:51:25
From previous event:
    at createMacApp
(ai-ico/package/elife-node/node_modules/electron-builder-lib/out/electron/electronMac.js:245:17)
    at
ai-ico/package/elife-node/node_modules/electron-builder-lib/src/electron/ElectronFramework.ts:56:11
    at Generator.next (<anonymous>)
From previous event:
    at beforeCopyExtraFiles
(ai-ico/package/elife-node/node_modules/electron-builder-lib/out/electron/ElectronFramework.js:167:17)
    at beforeCopyExtraFiles
(ai-ico/package/elife-node/node_modules/electron-builder-lib/src/electron/ElectronFramework.ts:131:14)
    at
ai-ico/package/elife-node/node_modules/electron-builder-lib/src/platformPackager.ts:223:13
    at Generator.next (<anonymous>)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)
    at processImmediate (timers.js:658:5)
<snip>

So I'm sticking with the latest version. I've edited the electron-builder code for now and removed the node_modules filter but that's probably the wrong thing to do.

Do you know why electron-builder is removing node_modules? I tried looking at the code but the commenting is very sparse and I can't figure it out.

@stale
Copy link

stale bot commented Aug 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the backlog label Aug 11, 2019
@florisporro
Copy link

Has this been fixed? If not, it's definitely something that should be addressed, it's a big headache.

@stale
Copy link

stale bot commented Oct 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the backlog label Oct 12, 2019
@TanninOne
Copy link

Not fixed yet

@stale
Copy link

stale bot commented Dec 13, 2019

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the backlog label Dec 13, 2019
@karlsnyder0
Copy link

I believe the developer actively ignores this issue. Nothing will happen. This has been open for more than 2 years, yet still no acknowledgement - nothing.

Unfortunately there's nothing else but electron-builder so we have to live with this garbage great tool.

I've updated my comment and included a patch that we will now use to work around this issue.

@stale
Copy link

stale bot commented Dec 18, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the backlog label Dec 18, 2020
@dmnsgn
Copy link

dmnsgn commented Dec 18, 2020

Is this still relevant?

Yes

@stale stale bot removed the backlog label Dec 18, 2020
@bayraak
Copy link

bayraak commented Jan 19, 2021

This issue still continues and no one is taking any action on it.

@tnrich
Copy link

tnrich commented Jan 27, 2021

@karlsnyder0 if your patch is working and not breaking other stuff maybe you could submit a PR for it anyways to get more attention to a possible fix for this pretty nasty issue? Just a thought.

PS. Thanks for the patch code (I hadn't heard about patch-package, what a sweet npm package)

@tnrich
Copy link

tnrich commented Jan 27, 2021

@karlsnyder0 I take back what I said. It doesn't appear your patch is working. I had to modify it slightly so it didn't break immediately:

patches/app-builder-lib+22.9.1.patch

diff --git a/node_modules/app-builder-lib/out/util/AppFileWalker.js b/node_modules/app-builder-lib/out/util/AppFileWalker.js
index 0f9cdad..933f43d 100644
--- a/node_modules/app-builder-lib/out/util/AppFileWalker.js
+++ b/node_modules/app-builder-lib/out/util/AppFileWalker.js
@@ -88,9 +88,9 @@ function createAppFilter(matcher, packager) {
 
   const filter = matcher.createFilter();
   return (file, fileStat) => {
-    if (!nodeModulesFilter(file, fileStat)) {
-      return false;
-    }
+    if (!['www/node_modules'].some(v => file.includes(v)) && !nodeModulesFilter(file, fileStat)) {
+       return false;
+     }
 
     return filter(file, fileStat);
   };
@@ -110,7 +110,7 @@ class AppFileWalker extends FileCopyHelper {
       // https://github.com/electron-userland/electron-builder/issues/1539
       // but do not filter if we inside node_modules dir
       // update: solution disabled, node module resolver should support such setup
-      if (file.endsWith(nodeModulesSystemDependentSuffix)) {
+      if (!['www/node_modules'].some(v => file.includes(v)) && file.endsWith(nodeModulesSystemDependentSuffix)) {
         return false;
       }
     } else {

In your version I was getting a str is undefined error so I changed str to file. Even with the modifications, your patch does not seem to fix the issue as I'm still getting the original module not found error:

Uncaught Exception:
Error: Cannot find module 'bio-parsers'
Require stack:
- /Users/tnrich/Sites/ove-electron/dist/mac/Open Vector Editor.app/Contents/Resources/app.asar/main.js

Any help from the maintainers on this issue would be appreciated.

Thanks!

@Julusian
Copy link
Contributor

I hit this issue and was forced to figure something out because of needing to update electron and electron-builder.
My workaround is to unpack the asar, repopulate the nested node_modules and then repack the asar.
https://github.com/bitfocus/companion/blob/master/tools/asar-repack.js https://github.com/bitfocus/companion/blob/master/package.json#L7

It has some issues, but works well enough for us.

@williamoliveira
Copy link

williamoliveira commented Feb 16, 2021

Is this error related?

    "electron-updater": "4.3.5",
    "electron": "11.2.3",
    "electron-builder": "^22.9.1",
(node:10890) UnhandledPromiseRejectionWarning: Error: Cannot find module 'fs-extra'
Require stack:
- /tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/out/AppUpdater.js
- /tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/out/BaseUpdater.js
- /tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/out/AppImageUpdater.js
- /tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/out/main.js
- /tmp/.mount_myappZZtDD/resources/app.asar/main.js
- 
    at Module._resolveFilename (internal/modules/cjs/loader.js:972:15)
    at Function.n._resolveFilename (electron/js2c/browser_init.js:249:921)
    at Module._load (internal/modules/cjs/loader.js:848:27)
    at Function.f._load (electron/js2c/asar_bundle.js:5:12738)
    at Module.require (internal/modules/cjs/loader.js:1032:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at _fsExtra (/tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/src/AppUpdater.ts:5:1)
    at AppImageUpdater.loadUpdateConfig (/tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/src/AppUpdater.ts:469:27)
    at Lazy.<anonymous> (/tmp/.mount_myappZZtDD/resources/app.asar/node_modules/electron-updater/src/AppUpdater.ts:141:43)
    at Lazy.get value [as value] (/tmp/.mount_myappZZtDD/resources/app.asar/node_modules/lazy-val/src/main.ts:18:25)

@Raynos
Copy link

Raynos commented Mar 26, 2021

This is still a frustrating issue today.

@somebeaver
Copy link

somebeaver commented May 17, 2021

It is well known that the "node_modules" folder cannot be renamed, so it is extremely frustrating that electron-builder makes such an opinionated decision by removing all nested node_modules folders from the app. You would think that there would be some flexibility with this option in particular, but no, everyone is forced to restructure their app or use an outdated version of electron-builder or use ugly patches. I do not understand how this is not a top priority issue.

@bedney
Copy link
Contributor

bedney commented May 18, 2021

So I'd be willing to make the patch that @tnrich has here into a PR for @develar. I've done other PRs that @develar has taken and are now in the codebase. Have folks found that patch to work for them? I'm willing to help out here. What do folks think?

@mmaietta
Copy link
Collaborator

Does anyone have a repro gist or repo that I could try locally? Something like electron-quick-start

@bedney, happy to review it

@bedney
Copy link
Contributor

bedney commented May 18, 2021

So it's been a while since I looked at this issue for my own purposes. I was able to workaround this by setting asar to false and using extraResources to explicitly copy the node_modules directory that I want into the proper place (which is less than ideal). I'm trying to get a handle on the real issue here which is, I think, that electron-builder is filtering out all node_modules directories in included packages (i.e subdirectories). And, furthermore, it is ignoring file pattern matching so that the behavior cannot be overridden.

Is this an accurate characterization of this problem? I'd like to help fix this, but if folks could confirm for me that this is the problem that they're seeing, we can go from there.

Thanks!

@somebeaver
Copy link

somebeaver commented May 18, 2021

@bedney

I think, that electron-builder is filtering out all node_modules directories in included packages (i.e subdirectories)

Exactly.

And, furthermore, it is ignoring file pattern matching so that the behavior cannot be overridden.

Also correct. This wouldn't be an issue if we could just add path/to/deep/node_modules/** to our package.json's files.

@somebeaver
Copy link

@mmaietta I just put together a minimal example: https://github.com/somebeaver/electron-builder-issue-3185

The readme has all the info needed. It's basically just the default create-electron-app but with the contents moved into a nested node_modules, which recreates the issues.

Please let me know if there's any issues with the repo.

@TanninOne
Copy link

Is this an accurate characterization of this problem? I'd like to help fix this, but if folks could confirm for me that this is the problem that they're seeing, we can go from there.

It's been a while since I concerned myself with this issue but this wasn't exactly what I experienced.
I got builds where the modules that did get bundled were an older version than what my main project required.
See my earlier message:

My project depends on semver@^5.6.0
A dependency I include (semvish) depends on semver@^4.6.3
electron-builder packages semver@^4.6.3 twice, in semvish/node_modules and the top-level node_modules

This means that in unfortunate cases the application might run but crash due to missing api or be exposed to security issues that were supposedly fixed.
Obviously this is two years old, maybe the behavior in more recent versions has changed again.

@bedney
Copy link
Contributor

bedney commented May 20, 2021

@TanninOne - thanks for the report, but I don't think that's what's going on here. I've tracked down the code where this bug is happening and it is coded to specifically omit nested node_modules directories (i.e. node_modules directories that are in submodules of the project). But I'll do some testing and see if I can repro your issue.

I have a patch in hand that I'm testing that seems to do what folks here want and I'm going to test it a bit more this morning before issuing the PR.

@bedney
Copy link
Contributor

bedney commented May 21, 2021

All, @somebeaver, @mmaietta -

My PR is located here: #5911.

First of all, thanks @somebeaver for putting together the excellent test case. I used it a lot when testing this.

So, this patch requires some explanation because it's my best attempt to solve a less-than-ideal situation. Currently, due to #1539, all node_modules directories of submodules (but not the main node_modules directory) were omitted from applications built with electron-builder. IMO, this particular feature should have been put behind a flag, but that was not done.

So that is part of what this patch does. I have introduced a new flag, includeSubNodeModules that is false by default (to keep the behavior of the past 3 years), but when switched to true, it will include the node_modules directories of all submodules.

Now, given @somebeaver's use case, I wanted to take it a bit further. There may be cases when you want to not include all submodules node_modules directories, but only some of them. This patch does that as well. If you leave the includeSubNodeModules flag set to false (again, the default), but you specify only certain node_modules directories in your files config property, those will be included as well, as per @somebeaver's example:

    "files": [
      "*/**",
      "src/embedded-submodule/node_modules/**"
    ],

So that's it.

Now, here's the thing about this patch: I am not a TypeScript programmer (been a JavaScripter forever), so I had to struggle a bit with some of the tricks I had to do to make TypeScript cooperate. I needed access to electron-builder's 'options configuration', but that wasn't being passed to the calls where I was so I had to do some 'under cover' private member stuff (and had to instruct the TypeScript compiler not to complain at me). I don't have a good enough knowledge of this codebase to feel like I can just go change APIs at will.

Also, it also looks like I'm getting a couple of test failures. So I could use some help to fix this patch (@mmaietta or others). Lastly, the includeSubNodeModules flag also needs to be added to the documentation.

That's all. I'm hoping we can push this forward and get it into the codebase.

Cheers,

  • Bill

@bedney
Copy link
Contributor

bedney commented Jun 2, 2021

A new version of this patch has been pushed and I'm actively working with the project maintainers to resolve any outstanding issues.

@bedney
Copy link
Contributor

bedney commented Jun 17, 2021

Further status update: I now have a patch that passes all of the tests (and I added a few tests to test this new capability). As soon as the maintainers pull this in, we should be good to go :-).

mmaietta pushed a commit that referenced this issue Jun 25, 2021
This patch adds the ability to package either all or a select set of *submodules* node_modules directories in an Electron app. Fixes #3185
@bedney
Copy link
Contributor

bedney commented Jun 25, 2021

All -

Thanks to @mmaietta this patch is now in the codebase trunk! Hopefully this fixes this longstanding issue. The patch implements the functionality as detailed in #3185 (comment).

If you find other issues with including submodules node_modules directories, please open a separate issue and mention me on that issue.

Cheers!

  • Bill

@mmaietta
Copy link
Collaborator

Unfortunately, this caused this nasty bug to appear: #6045
Basically 22.11.8-9 only work for solving this ticket's use case, but breaks previous ones
Just an FYI here in case you're adopting the latest version
Working on getting it resolved asap

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

Successfully merging a pull request may close this issue.