Rules_nodejs: Create a symlink for a package's BUILD.bazel file on "link:" node modules

[mistic]

🐞 bug report

Affected Rule

yarn_install or npm_install

Is this a regression?

No

Description

Currently when using managed dependencies if we declare a link: in our package.json to a local package the resulting generated BUILD.bazel file would be a concatenation between a common BUILD.bazel content for a node_module plus the content of the BUILD.bazel file in the linked package source code, if any. That behaviour creates a couple of problems and make the rules_nodejs not compliant with the common node development environemnt.

That would be solvable if we create a symlink for the package source BUILD.bazel on the npm workspace instead of generating a file. If the package source doesn't contain a BUILD.bazel we can still create the symlink and log a warning to let the user know he must create a BUILD.bazel file in order to use link: packages as node_modules. A possible implementation for this feature would be to look for symlinked modules at https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/npm_install/generate_build_file.ts#L475 and filter those out to be processed in another function responsible for creating the symlinks for ${realpath}/BUILD.bazel. In order to make that operation more user friendly the BUILD.bazel created by the user should only be needed to expose a target with the same name of the package so the implementation of that feature needs to rework a little what is being generated by https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/npm_install/generate_build_file.ts#L964

🔬 Minimal Reproduction

There is none provided. A simple bazel setup with managed dependencies and a link: on the package.json for some local package to be symlinked as a node_module would be enough.

🔥 Exception or Error

None

🌍 Your Environment

Operating System:

  
macOS 10.15.6
  

Output of bazel version:

  
Bazelisk version: v1.5.0
Build label: 3.3.0
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Wed Jun 17 12:59:56 2020 (1592398796)
Build timestamp: 1592398796
Build timestamp as int: 1592398796
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
[email protected]
@bazel/[email protected]
  

Anything else relevant?

No

[mistic]

@migueloller firstly pointed a solution like that and I've discussed the details offline with him!

[migueloller]

@mistic, thanks for opening this issue! I wanted to mention a few things:

• Yarn does symlinks when using link: but NPM does symlinks when using file:. Lerna will do symlinks for workspaces. So instead of worrying about how to handle specific dependency protocols in package.json, I would suggest we focus on handling the case where https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/npm_install/generate_build_file.ts encounters a symlink.
• Instead of creating a symlink to the BUILD file, I would suggest to simply create a symlink to the realpath of the encountered symlink. For example, if a symlink is encountered in node_modules/lib -> ../packages/lib then instead of Bazel generating a BUILD file at npm/lib/BUILD, it could just do npm/lib -> ./relative/path/to/packages/lib.
• The reason why concatenating the BUILD files results in unexpected behavior is that the files that https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/npm_install/generate_build_file.ts encounters during traversal and the BUILD file contents (concatenated) could change and diverge from the generated BUILD file. By doing a symlink, it prevents this divergence.

[mistic]

@migueloller thanks for detailing further the explanation! Your first point was already in consideration in what I wrote before I believe. I agree with the second one and I believe is even better then what I suggested above. Regarding your third and last point, all clear, is the correct explanation for the problem we face here! 🎉

[alexeagle]

This sounds reasonable to me, we never tried having link: or file: URIs in our package.json. Thanks for picking this up!
If you want to break this into a few steps, you could start by introducing a draft PR with a repro so it's easier for us to play with the problem and see what red CI has to say about it.

[mistic]

One point we forgot to mention here but that is important. We should create documentation around what we do expect from the users in respect to the content of the BUILD.bazel files for the symlinked node_modules. In the current architecture it should be defining js_library both for pkg__contents and pkg__typings as the default target for every node_module contains those. @alexeagle do you think we should also challenge that and try to condense every node_module file in the default target removing __contents and __typings?

[migueloller]

Yeah, I do think that would be a problematic API. Ideally it would be as follows:

• If a BUILD file is found, there is an expectation that there is a rule with the same name as the Bazel package, i.e., @npm//pkg:pkg, which of course can just be referenced as @npm//pkg (instead of pkg__typings and pkg__contents).
• If a BUILD file is not found, then a warning is logged saying that there was a symlink so a BUILD file wasn't generated and a short explanation as to why generating it would be a bad idea.

[mistic]

@migueloller there is indeed a target created for @npm//pkg:pkg but it contains @npm//pkg:pkg__contents as a dep and the way we are currently generating the BUILD files for the node_modules is by referencing @npm//pkg:pkg__contents. @alexeagle was that created with a reason to reduce the number of the input files in some cases or there is no reason and we can just convert it to always reference @npm//pkg:pkg only and stop splitting into __contents and __typings?

[mistic]

Alright just another detail I remember and I want to left documented here. When processing the node_modules and looking for symlinks we should only consider to symlink in the case the realpath of the found symlink is contained inside the workspace otherwise we should consider it a node_module. pnpm creates the node_modules by using symlinks so in case we wanna support it in the future, it is better to design the solution right now.

[github-actions[bot]]

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!

[mistic]

@gregmagolan do you have any input about this one?

[staeke]

I'm also very interested in this one. Also potentially able to pick up work if needed. @mistic @migueloller

[mistic]

@staeke that is something definitely under my radar as it is a feature I will need to successfully complete the migration into bazel that I'm currently working on. However I still have other things to complete before looking into this one. If you have time and want to start looking into this one that will be much appreciated 😃

[mistic]

I've spent sometime looking at this and I've came up with https://github.com/mistic/rules_nodejs/commit/45be2f636e88f5817bdd9a9fe7ea1217ebaf9a73

I believe it is working as intended but we might still do something on it before getting into its final shape