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

fix: Edge case where ts_proto_library depends on proto_library(strip_import_prefix=...) #759

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amari
Copy link

@amari amari commented Jan 9, 2025

This PR fixes #476, an edge case where a ts_proto_library depends on a proto_library(strip_import_prefix=...).

For example, the file proto/myorg/routeguide/v1/route.proto:

  • can be declared via myorg.routeguide.v1 instead of proto.myorg.routeguide.v1, and
  • can be imported using the path myorg/routeguide/v1/route.proto instead of proto/routeguide/v1/route.proto

Previously, ts_proto_library was not providing a complete set of imports to protoc.

After fixing the import problem, building failed b/c .d.ts and .js files were being written to the wrong location.

I needed helper functions from @protobuf//bazel/common:proto_common.bzl, but since this repository still depends on the deprecated @rules_protobuf, I just copied them.

However, they're governed by Google's BSD-like license. I would like to refactor it into a "third-party" directory, but none exists and I saw no documentation/guidelines about how to go about it. I also plan on adding an example after getting feedback on this.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: no
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

Fixes edge case where ts_proto_library depends on proto_library(strip_import_prefix=...)

Test plan

  • TODO: will include test after I get feedback on whether a "third-party" directory is even necessary vs attribution.

For now, I've removed the test-case.

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

aspect-workflows bot commented Jan 9, 2025

Test

3 test targets passed

Targets
//examples/proto_grpc:main [k8-fastbuild]                        201ms
//examples/proto_grpc:status_ts_proto.copy_0_test [k8-fastbuild] 55ms
//examples/proto_grpc:status_ts_proto.copy_1_test [k8-fastbuild] 52ms

Total test execution time was 308ms. 160 tests (98.2%) were fully cached saving 15s.


Buildifier      Format

@amari
Copy link
Author

amari commented Jan 9, 2025

I ran into these same testing failures locally.

They were resolved by running: bazel run //examples/proto_grpc:status_ts_proto.copy_1 and bazel run //examples/proto_grpc:status_ts_proto.copy_0.

I didn't include them in the PR b/c the changes seemed out of scope.

EDIT:

The tests failed b/c previously the comments from examples/proto_grpc/status.proto were not copied over by protoc to examples/proto_grpc/status_pb.d.ts and examples/proto_grpc/status_connect.d.ts.

I've added the generated .d.ts files to the PR.

@amari amari force-pushed the fix-strip-import-prefix branch from 1b5bf74 to ce58380 Compare January 10, 2025 16:45
@amari
Copy link
Author

amari commented Jan 10, 2025

@kormide @jbedard @alexeagle

@amari amari changed the title fix: ts_proto_library with strip_import_prefix in proto_library fix: Support for proto_library targets that use strip_import_prefix Jan 21, 2025
@amari amari changed the title fix: Support for proto_library targets that use strip_import_prefix fix: Edge case when ts_proto_library depends on proto_library(strip_import_prefix=...) Jan 21, 2025
@amari amari changed the title fix: Edge case when ts_proto_library depends on proto_library(strip_import_prefix=...) fix: Edge case where ts_proto_library depends on proto_library(strip_import_prefix=...) Jan 21, 2025
@amari
Copy link
Author

amari commented Jan 22, 2025

@jbedard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ts_proto_library does not work with import_prefix and strip_import_prefix in proto_library
2 participants