Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guidance around exporting macros with strict_visibility = True #2393

Closed
dgp1130 opened this issue Jan 10, 2021 · 5 comments
Closed

Guidance around exporting macros with strict_visibility = True #2393

dgp1130 opened this issue Jan 10, 2021 · 5 comments
Labels
Can Close? We will close this in 30 days if there is no further activity

Comments

@dgp1130
Copy link
Contributor

dgp1130 commented Jan 10, 2021

💭 guidance / best practices

Description

Consider an NPM package which exports a Bazel macro that generates some code. Then imagine that the generated code uses a dependency. For example:

load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")

def my_binary(name):
    nodejs_binary(
        name = name,
        # ...
        data = ["@npm//yargs"],
    )

Running this directly in my library workspace is just fine, because I have listed a dependency on @npm//yargs via my package.json and installed it.

However, when a user of my library calls this macro, the same nodejs_binary() gets generated in their own workspace. They may not have a dependency on yargs and strict visibility may reject the dependency.

See this minimal reproduction.

Describe the solution you'd like

I'm unclear on how to handle this particular kind of issue. The only obvious solution is to make users of the library set strict_visibility = False, which is somewhat of a nuclear option. It would also apply to all workspace that use a macro from a dependency which uses a transitive dependency, which I imagine could be quite a common use case.

Ideally, this would "just work" somehow, considering the dependency is coming from a macro in the library project which has explicitly listed the dependency and that should be enough to allow it. Unfortunately Bazel just doesn't work that way and I don't see a way that could be made to work without serious changes in Bazel itself.

Beyond that, one idea I can think of would be for rules_nodejs to emit a @npm//yargs:yargs_nonstrict target, which is equivalent to @npm//yargs but with visibility = ["//visibility:public"]. That would allow macros to depend on an unstrict version and trust that the transitive dependency exists without requiring users to disable strict visibility altogether. Uses of this target are effectively opting-out of strict visibility explicitly on a per-target basis, so default usage would still be strict and keep most of the benefits of the flag.

While that suggestion would be more of a feature request, I'm mainly looking for general guidance on how to deal with this kind of problem. I could not find any documentation or resources which call out this particular issue. I assume some @bazel/* package must have their own dependencies which get exported to user projects via macros. I tried looking through a couple but couldn't find any obvious workarounds to this problem. Is there a strategy for working around strict visibility for NPM packages that export custom macros with transitive dependencies?

Describe alternatives you've considered

A workaround that could work for some instances is to vendor all transitive deps. Either bundle the binary or inline yargs directly in the NPM package. But doing that basically drops all the benefits of using npm_install() for a library project, since we can't expect any reference @npm//some-other-package/... to ever pass strict visibility checks.

Another manual workaround I can think of would be for a library workspace to generate a @npm//my_library/deps:yargs target, which is an alias to @npm//yargs, then depend on the former instead of the latter. I think that would be enough for this case, however I don't think it composes well. If someone else made a wrapper library of mine and published it, they would need to do the same deps trick, except my macro would assume @npm//my_library/deps:yargs is strict visibility compatible, when it would actually need to use @npm//wrapper_library/deps/my_library:yargs. I would need to allow users to inject all my dependencies, that way the wrapper library could set the wrapped labels. That would break encapsulation pretty hard and is definitely not a great approach IMHO.

@dgp1130
Copy link
Contributor Author

dgp1130 commented Mar 5, 2021

Are there any thoughts about how this particular problem should be solved? I noticed the "help wanted" tag on this, is that because the desired behavior is adding a @npm//pkg:pkg__nonstrict target with //visibility:public as suggested and we just need someone to do the work? I could probably help with that aspect if we've at least decided this is the right solution to the problem (would also appreciate some pointers to the relevant code).

@alexeagle
Copy link
Collaborator

yeah if you're distributing Bazel rules that depend on npm packages, then you should distribute those rules via npm like we do with @bazel/typescript. This is the only way your rule can declare its dependency on yargs rather than assume the user will depend on it themselves

@dgp1130
Copy link
Contributor Author

dgp1130 commented Mar 7, 2021

I'm not sure I'm understanding. Is strict visibility supposed to work for Bazel macros as long as they are installed via NPM managed dependencies? Does @bazel/typescript do something unique in this regard?

I noticed that ts_library() has a dependency on @npm//typescript__typings. That works because typescript is a peer dep that must be installed by the consumer, but I don't see how that could work if typescript was a direct dependency. It also doesn't work if another Bazel library were to wrap ts_library() in a custom macro and provides its own typescript install. Then a user project would not have a direct dependency on typescript, and using the ts_library() wrapper would not be able to depend on @npm//typescript__typings due to strict deps.

Is the intended solution to use peer deps instead of regular dependencies, or is there something else I'm missing here?

@github-actions
Copy link

github-actions bot commented Jun 5, 2021

This issue has been automatically marked as stale because it has not had any activity for 90 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 github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Jun 5, 2021
@github-actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity
Projects
None yet
Development

No branches or pull requests

3 participants