Open hipstersmoothie opened 2 years ago
This actually means if you use any tool that makes sym links to node_modules won't work. Just encountered this again when trying to use yarn's link:
in my mono repo
I also encountered this when switching https://github.com/NicBOMB/Vortex to pnpm and using workspace linking (which is incredibly useful). I was looking at implementing a fix and encountered some issues with the queue's structure in the nodeModuleCollector (invoked by ./app-builder-bin/linux/x64/app-builder node-dep-tree --dir "<path to repo>"
after building).
Right now output of this command may resolve to the wrong node_modules
even if the dependency definitely exists in the desired node_modules
( #20 ) and I don't see a simple fix for symlinking.
The structure of the queue assumes all dependencies may fallback to the main project's node_modules
as if hoisting all packages. This behavior is valid only for yarn classic or total hoisting in other package managers. The algorithm used is also not strictly accurate to the CJS resolution algorithm and/or ESM resolution algorithm (even if symlinks were to be evaluated).
Specifically related to #20 the provided algorithm uses a subtractive step:
https://github.com/develar/app-builder/blob/4e2aa6a12e2bc3d31ec0d01d661fb3a4d65248ff/pkg/node-modules/nodeModuleCollector.go#L117
rather than an additive one (see the NODE_MODULES_PATHS
pseudo-code from the node docs on CJS resolution). This discrepancy alone might not be a problem. However, when using a dedicated 2 package.json setup (split dev and app package manifests), the queue algorithm will eventually step backwards into the main project's node_modules
and specifying the app directory (as is done by electron-builder and seen in #20 ) does not prevent this, which is particularly unwanted.
New package managers which support comprehensive dependency linking are specifically more appealing for this setup because they can dramatically reduce duplicate packages and installation times. Using a subtractive path construction step in the dependency tree resolver does trivially avoid dependency cycles but is causing the desired package paths to be missed and is notably incompatible with evaluating symlinked packages.
The resolver needs to more strictly adhere to the CJS/ESM resolution algorithm(s) AND evaluate symlinks.
I saw @mmaietta recommending creating an issue here with details, but I think a PR to electron-builder
in the future may be a better approach, especially as detecting the packageManager used by a project becomes simpler. However, that would be a major change to dependency packaging for electron-builder.
@develar is there a particular reason for keeping this node_modules
dependency resolution algorithm separate from electron-builder? (other than needing to replace electron-builder
's tooling implemented specifically for app-builder-bin
already)
I think a portable node module implementation of this node_modules
tree resolver algorithm (with fixes for symlinks and isolated dependencies) or the use of another library would be better instead.
@mmaietta @develar thoughts/feedback/criticism?
I created a PR to try to solve the problem of electron-builder and symlinks. This fix aims to support the node_modules structure of pnpm, and should also be able to handle the monorepo situation. Can someone review it for me?
The basic idea is to find the real path of the symlinks, and then recursively find node_modules/[pkg]
https://github.com/develar/app-builder/pull/89
@mmaietta @develar
Hi @rxliuli , I don't manage this project and also don't know Golang to be able to help out here.
It would be awesome to work with symlinks properly. PNPM provides many advantages over NPM & Yarn and popular enough to support it!
it's merged,
now we need new release of this package right?
Even though #89 has been merged, there are still some tasks that need to be completed:
app-builder-lib
in electron-builder
repo)This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 30 days.
Go away stale bot, you're not welcome here
Deleted the stale bot from the repo 😅
is there any update on this? I'm still having issues with building an MSI with the latest electron-builder in a PNPM workspace
Currently this package doesn't handle sym links very well and as a result is incompatible with pnpm.
https://github.com/electron-userland/electron-builder/issues/6792#issuecomment-1098555140
For pnpm support it would be great if symlinks in node_modules were parsed as well