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

Add TypeScript types output to nodejs_proto_* rules #103

Merged
merged 1 commit into from
Feb 21, 2021

Conversation

alexeagle
Copy link
Contributor

Makes the nodejs_proto_library rule provide DeclarationInfo so it can be a dependency of a ts_project or ts_library rule

Makes the nodejs_proto_library rule provide DeclarationInfo so it can be a dependency of a ts_project or ts_library rule
@alexeagle
Copy link
Contributor Author

Hi @aaliddell what's your process for accepting contributions here?

@aaliddell
Copy link
Member

Hi, sorry for the delay on this one and @ash2k’s, it has been a busy couple of weeks. These are on my todo list when I get a chance

@aaliddell
Copy link
Member

aaliddell commented Feb 7, 2021

Just a FYI: CI fell over when Bazel 4 was released in Jan. I'm getting CI back to green, then hopefully these can be applied more cleanly.

With regard to this particular PR, am I correct in saying this will just produce typescript files alongside the normal .js files? I assume this doesn't yet create the provider that you need in the top comment? I've been away from JS/TS for a couple of years, so perhaps I’m missing something 🤔

@aaliddell aaliddell mentioned this pull request Feb 9, 2021
@aaliddell aaliddell merged commit b5d92e5 into rules-proto-grpc:master Feb 21, 2021
@pshields
Copy link

Very excited to see this update. :) Much appreciation to @aaliddell and @alexeagle. I just tried out using the js_grpc_node_compile and js_grpc_node_library rules from rules_proto_grpc 3.0, and while I do see those targets produce the .d.ts files as expected, when I try to use them as deps to a ts_project, I get errors in both cases that those targets do not have mandatory providers: 'DeclarationInfo' or '_ValidOptionsInfo'. Am I missing something?

@aaliddell
Copy link
Member

aaliddell commented Feb 22, 2021

Ah I see the issue, js_proto_library will correctly produce the DeclarationInfo provider, but js_grpc_node_library will not as it produces a directory output that can't be inspected for .ts files.

Could you test the js-decl-info branch?

http_archive(
    name = "rules_proto_grpc",
    urls = ["https://github.com/rules-proto-grpc/rules_proto_grpc/archive/31d5a99f3a31579e2fc9b92adab9d8d0f9f19830.tar.gz"],
    sha256 = "444409acf527947c0b27dc62adba570303049884cfc9d924d40e31e78552f9e8",
    strip_prefix = "rules_proto_grpc-31d5a99f3a31579e2fc9b92adab9d8d0f9f19830",
)

If that works I'll merge it in.

@pshields
Copy link

I updated my WORKSPACE to use that new http archive. My js_grpc_node_compile-based target still complains about the missing mandatory DeclarationInfo provider (when passed in as a dep to a ts_project-based target), while my js_grpc_node_lib-based target fails to build at all, with the following error:

bazel build //src:service_js_grpc --verbose_failures
INFO: Analyzed target //src:service_js_grpc (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: [home dir]/.cache/bazel/_bazel_patrick/3aa8252db114c8fb4bc0778f64c317ff/external/js_modules/ts-protoc-gen/bin/BUILD.bazel:9:14: Middleman _middlemen/external_Sjs_Umodules_Sts-protoc-gen_Sbin_Sprotoc-gen-ts.sh-runfiles failed: missing input file 'external/js_modules/node_modules/ts-protoc-gen/CHANGELOG.md', owner: '@js_modules//:node_modules/ts-protoc-gen/CHANGELOG.md'
Target //src:service_js_grpc failed to build
ERROR: [home dir]/.cache/bazel/_bazel_patrick/3aa8252db114c8fb4bc0778f64c317ff/external/js_modules/ts-protoc-gen/bin/BUILD.bazel:9:14 Middleman _middlemen/external_Sjs_Umodules_Sts-protoc-gen_Sbin_Sprotoc-gen-ts.sh-runfiles failed: 97 input file(s) do not exist
INFO: Elapsed time: 0.084s, Critical Path: 0.01s
INFO: 3 processes: 3 internal.
FAILED: Build did NOT complete successfully

Would you prefer that I log an issue to capture further discussion? I could also try to make a small example repo to reproduce the issue, if that would be helpful.

@aaliddell
Copy link
Member

I updated my WORKSPACE to use that new http archive. My js_grpc_node_compile-based target still complains about the missing mandatory DeclarationInfo provider (when passed in as a dep to a ts_project-based target), while my js_grpc_node_lib-based target fails to build at all, with the following error:

bazel build //src:service_js_grpc --verbose_failures
INFO: Analyzed target //src:service_js_grpc (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: [home dir]/.cache/bazel/_bazel_patrick/3aa8252db114c8fb4bc0778f64c317ff/external/js_modules/ts-protoc-gen/bin/BUILD.bazel:9:14: Middleman _middlemen/external_Sjs_Umodules_Sts-protoc-gen_Sbin_Sprotoc-gen-ts.sh-runfiles failed: missing input file 'external/js_modules/node_modules/ts-protoc-gen/CHANGELOG.md', owner: '@js_modules//:node_modules/ts-protoc-gen/CHANGELOG.md'
Target //src:service_js_grpc failed to build
ERROR: [home dir]/.cache/bazel/_bazel_patrick/3aa8252db114c8fb4bc0778f64c317ff/external/js_modules/ts-protoc-gen/bin/BUILD.bazel:9:14 Middleman _middlemen/external_Sjs_Umodules_Sts-protoc-gen_Sbin_Sprotoc-gen-ts.sh-runfiles failed: 97 input file(s) do not exist
INFO: Elapsed time: 0.084s, Critical Path: 0.01s
INFO: 3 processes: 3 internal.
FAILED: Build did NOT complete successfully

You'd definitely need js_grpc_node_library rather than js_grpc_node_compile, since the latter just makes the files without bundling into a library to make the providers.

Would you prefer that I log an issue to capture further discussion? I could also try to make a small example repo to reproduce the issue, if that would be helpful.

Sure, that'd be useful

@pshields
Copy link

Thanks for the clarification about js_grpc_node_compile not being intended to provide a DeclarationInfo. I've logged #113, scoped to js_grpc_node_library, and provided a relatively minimal repro. The repro also reproduces the input files do not exist error when updating the WORKSPACE rules_proto_grpc reference to point to the above-mentioned js-decl-info branch.

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.

3 participants