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

NodeJS require paths are relative to the proto/grpc rule #107

Closed
Tracked by #140
mmatheson opened this issue Feb 10, 2021 · 6 comments
Closed
Tracked by #140

NodeJS require paths are relative to the proto/grpc rule #107

mmatheson opened this issue Feb 10, 2021 · 6 comments
Labels
enhancement New feature or request lang-js JavaScript rules specific resolved-next-release Code to resolve issue is pending release
Milestone

Comments

@mmatheson
Copy link

The require path for the _pb.js files generated by the nodejs_* rules is inconsistent with other language implementations.

In the cpp example:
#include "example/proto/routeguide.grpc.pb.h" matches the location of the routeguide proto in the repo (example/proto/routeguide.proto).

In the nodejs example: require('routeguide/routeguide_pb/example/proto/routeguide_pb.js') appears to be {path/to/nodejs_grpc_library}/example/proto/routeguide.proto. This inconsistency leads to long, hard to parse require paths, and obscures the actual location of the .proto file when reading source code.

@aaliddell
Copy link
Member

This is because the cc_library rule provides a mechanism for expressing the include root with the includes attribute, which allows us to hide the generated prefix. With js_library, we are only able to provide the package name, which in this case is routeguide. The path is then generated statically for any srcs: https://github.com/bazelbuild/rules_nodejs/blob/2c2cc6eec42b0f4f805fb3063920107b78f821fd/internal/js_library/js_library.bzl#L226

To fix this, we'd either need to have support from rules_nodejs for producing a package that has a path pointing into the routeguide_pb directory (similar to prefix stripping in other rules) or otherwise wrap/patch the produced LinkablePackageInfo provider to amend the path.

In either case, the import would become require('routeguide/example/proto/routeguide_pb.js'), not just require('example/proto/routeguide_pb.js'), since a package name is needed regardless.

@aaliddell aaliddell added the bug Something isn't working label Feb 11, 2021
@aaliddell aaliddell added enhancement New feature or request lang-js JavaScript rules specific and removed bug Something isn't working labels Feb 11, 2021
@aaliddell aaliddell added the blocked-external Waiting on external issue/release label Feb 21, 2021
@aaliddell
Copy link
Member

Pending on bazelbuild/rules_nodejs#2482

@abrmv
Copy link

abrmv commented Mar 10, 2021

Experiencing the same issue with js_grpc_web_compile - the only workaround I found is to hack the prefix_path attribute to point to the workspace root e.g.

js_grpc_web_compile(
    name = "echo_proto_web",
    protos = [":echo_proto"],
    visibility = ["//visibility:public"],
    prefix_path = "../../.."
)

@aaliddell
Copy link
Member

There will hopefully be a fix for this coming: bazelbuild/rules_nodejs#2519

To maintain backwards compatibility, this'll have to be an opt-in mode at least for the 3.x.x cycle.

@aaliddell
Copy link
Member

Support for the required prefix stripping feature in rules_nodejs landed in 3.2.3, meaning I can begin looking to implement this.

@aaliddell aaliddell removed the blocked-external Waiting on external issue/release label Mar 25, 2021
@aaliddell aaliddell added this to the 4.0.0 milestone Sep 14, 2021
@aaliddell aaliddell mentioned this issue Sep 16, 2021
10 tasks
@aaliddell
Copy link
Member

This will be fixed in 4.0.0. The require path will by default have the name_pb removed, with an option to use the old behaviour

@aaliddell aaliddell added the resolved-next-release Code to resolve issue is pending release label Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lang-js JavaScript rules specific resolved-next-release Code to resolve issue is pending release
Projects
None yet
Development

No branches or pull requests

3 participants