Preserve symlinks by default when running node under Bazel

[mrmeku]

Background information

Preserving symlinks == the symlink is treated as the real path.
Expanding symlinks == the symlink is discarded in favor of the real path.

Node by default does not preserve symlinks. They tried to change this behavior in Node 6 (nodejs/node#5950) but that backfired with non-trivial breakages and they reverted it soon and put behind a --preserve-symlinks flag (nodejs/node#6537).
(See jestjs/jest#5356 (comment) for where I derived this explanation)

The problem (packages like Jest which walk the file tree to pickup files)

Jest has a crawling phase in which is crawls the file system using a glob pattern to pickup/transform test files. Jest is not alone is following such a semantic. Many npm packages allow passing in a glob pattern to perform similar work.

The problem occurs when tools use functions such as fs.realpath (or similar) to expand symlinks. These symlinks, when expanded will resolve to the location of these files relative to the execution root. In general, I don't think this is the behavior we want. Both nodejs_binary and nodejs_test use runfiles to symlink from exec root into a runfiles directory. So far as the program being executed is concerned, no other directories should exist. In fact, we already try to enforce that behavior by setting BAZEL_PATCH_ROOT=$(dirname $PWD) https://github.com/bazelbuild/rules_nodejs/blob/3321ed5fce23ed371ae3d8291b76f6d057a7f28f/internal/node/node_launcher.sh#L208

Proposed solution

In general I think that by default we should execute node with the preserve-symlinks flag set to true. Additionally, node should also be executed with preserve-symlinks-main set to true (See the bazel specific issues filed here for why this flag was introduced: nodejs/node#19383 (comment))

[alexeagle]

The issue title says enforce, but I think you're really suggesting we flip the default?

[Toxicable]

What would be the solution for packages like Jest be?
Should we put out PRs to adjust their behaviour to support preserve-symlinks = True

[mrmeku]

There's three solutions I can think of:

1. Put out PRs to make libraries do the right things when preserve-symlinks is true
2. Stop using runfiles for nodejs_binary/nodejs_test such that symlinks are not a concern
3. Patch fs in such a way that we can force symlink preservation. (IE patch fs.stat/fs.realpath/fs.realpath.native to pretend the real path is the symlink path

I think the ideal solution is #2 for compatibility reasons. Putting out Bazel compat PRs is time consuming and onerous. #3 probably has unforseen consequences. We have are already in a situation where we need to support #2 for npm_package_bin. It would probably be beneficial to execute nodejs_binary/nodejs_test the same way as npm_package_bin

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[github-actions]

This issue was automatically closed because it went two weeks without a reply since it was labeled "Can Close?"