feat: create symlink for build files present on node modules installed with relative paths

[mistic]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature (please, look at the "Scope of the project" section in the README.md file)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

What is the current behavior?

Currently if a local module installed with npm contains a BUILD file we generate the usual BUILD file content and just append the one found on the BUILD file.

Issue Number: #2131

What is the new behavior?

With this PR the BUILD file inside an installed local module will be symlinked instead of generated and get its content appended.

Does this PR introduce a breaking change?

• Yes
• No

It is really unlikely that the current behaviour is being useful to someone who relies on having a BUILD file in a local installed npm module as it introduces a race condition. However, the migration path would be to rewrite their BUILD files inside such modules to contain the expected generated targets by npm_install/yarn_install rules like *-module__contents or *--module__files and also add the extra targets they have previously defined on the BUILD file.

HINT: In order to don't have to manually writ *-module__contents or *--module__files we can remove the BUILD file, run the npm_install/yarn_install rules and just grasp the generated BUILD file content.

Other information

[alexeagle]

Awesome, this looks good to me. Thanks! I think I agree that no one really should rely on the current behavior but @gregmagolan added that and has a better memory than I do.

[gregmagolan]

This looks great. I read through the issue and the code and agree that this is the right thing to do if the node_module is a symlink to inside the workspace. It's a clever way to consume 1st party deps. The only thing I'd add is to update the yarn_install & npm_install docs to document this feature & the reasoning.

As an aside, why use this method instead of js_library with package_name to link 1st party deps? End result of a symlink in node_modules to the 1st party dep seems the same tho the js_library with package_name would use the linker to achieve it.

[mistic]

@gregmagolan Thanks for reviewing this as you are one of the few people well positioned and with enough knowledge to validate the idea. Regarding your question, js_library with package_name could be used to solve almost every situation. The problem arises when a local package consumed by the monorepo also gets published into the npm registry and starts being used by other node_module that you also have installed in your monorepo. That will cause a race condition that will make you not being able to use the linked node_module correctly as the generated BUILD file is not updated yet. If you run npm/yarn install a second time it will succeed because the BUILD file on the second run gots updated. Fixing the legacy behaviour of appending BUILD file contents and link instead will empower us to cover all the situations we could end up having in a node environment.

[mistic]

@alexeagle I've also added some content both to the yarn_install and npm_install rules about that new behaviour. Can you take a look and review it please?

I've also squashed the commits into a single one with a meaningful commit message for the feature.

[alexeagle]

I'm still not sure this use case of bazel -> bazel dependency makes sense, even given your problem description I still think you should have a js_library with package_name as the dependency of any Bazel code. That should be orthogonal to whether some non-Bazel dependencies in your repo still depend on the npm-built package.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.