fix(): remove _repository_args from nodejs_binary

[dymart]

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?

nodejs_binary requires _repository_args and has a dependency on @nodejs

What is the new behavior?

removes the need for @nodejs for _repository_args

Does this PR introduce a breaking change?

• Yes
• No

Other information

[gregmagolan]

Lets remove it out of nodejs/repositories.bzl and nodejs/private/nodejs_repo_host_os_alias.bzl in this PR along with the preserve_symlinks attribute of node_repositories

[gregmagolan]

Thinking about the current default which is to add --preserve-symlinks. I think we should just go with the nodejs default of not setting that flag and the releases notes for 5.0 should be for users to add --preserve-symlinks to their args with

nodejs_binary(
    ...
    args = ["--node_options=--preserve-symlinks"],
)

if they want it set.

@alexeagle do you think that would be too breaking? I suppose we'll see how CI is if we flip the default in RNJ.

User's would have to do this for all their nodejs_binary, nodejs_test targets. Suggestion in release notes could be to use a defaults macro.

If we don't want to flip the default, we'll need to figure out some API to have this thing be on by default outside of the standard args and templated_args.

There is some precedent with the default_env_vars, but default_args seems wrong as args are harder to override than env vars. That leaves making a special case attribute of preserve_symlinks that defaults to True for 5.0.

[dymart]

I've left "--preserve-symlinks" in for right now to pass the tests. I'll keep trying to get things working with the flag removed and then flip it when that works.

[alexeagle]

I think this may be a use case for a custom flag.

https://docs.bazel.build/versions/main/configurable-attributes.html#custom-flags

So users would bazel build --@rules_nodejs//nodejs:default_args="--something"
the default configuration would have the --preserve-symlinks in there.

[dymart]

Great thanks for the recommendation @alexeagle I'll look into using a custom flag

[dymart]

Hey @alexeagle just wanted to follow up in the example you gave it has these flags living in the nodejs folder. Would these flags still be used in the internal/node/node.bzl and passed along to launcher.sh through template variables. Or is there a different way you were thinking that these flags would be integrated in.

[alexeagle]

I would expect the custom flag in the nodejs/ folder like that example, yes. Can then be used in internal/node/node.bzl or anywhere else that wants to reference them

[alexeagle]

if this is the default, then why did you have to add it explicitly to a bunch of examples? seems wrong, I'd expect the examples not to change, since those illustrate the changes users would have to make as well.

[dymart]

The flags were added to the examples to see how things would work when the default was changed. If we wanted to go with the nodejs default. As mentioned earlier in the comments on the pr. I'm happy to remove them from this pr though and reevaluate when we want to change the default.

[gregmagolan]

I think keeping the default to match what node_repositories() had makes sense for 5.0. We'd want to do some more due diligence before we flip the default.

[gregmagolan]

I think we should just open this up and let users can set whatever default they want. Guessing they can set a defaults in their .bazelrc file with this improved approach?

[gregmagolan]

Lets add a real flag here to better illustrate what it does. Maybe pass two such as
"--preserve-symlinks --no-warnings" to show that you can pass multiple. A drop a link to https://nodejs.org/api/cli.html for reference.

[gregmagolan]

🌮