Handle symlinks

[hipstersmoothie]

Currently this package doesn't handle sym links very well and as a result is incompatible with pnpm.

electron-userland/electron-builder#6792 (comment)

For pnpm support it would be great if symlinks in node_modules were parsed as well

[hipstersmoothie]

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

[NicBOMB]

the issue

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 current algorithm

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:

app-builder/pkg/node-modules/nodeModuleCollector.go

Line 117 in 4e2aa6a

nodeModuleDir, err = findNearestNodeModuleDir(getParentDir(getParentDir(nodeModuleDir)))

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.

my feedback

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?

[rxliuli]

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]

#89

@mmaietta @develar

[mmaietta]

Hi @rxliuli , I don't manage this project and also don't know Golang to be able to help out here.

[psd-coder]

It would be awesome to work with symlinks properly. PNPM provides many advantages over NPM & Yarn and popular enough to support it!

[miaulightouch]

it's merged,

now we need new release of this package right?

[miaulightouch]

Even though #89 has been merged, there are still some tasks that need to be completed:

1. Fix package paths, similar to what was done in fix: Incorrect 'node_modules' path for the dependencies. #93
2. Rebuild the symlink structure to the destination path instead of simply copying packages. (require some modify to app-builder-lib in electron-builder repo)

[github-actions]

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.