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

import_style=typescript with Bazel? #550

Closed
prestonvanloon opened this issue May 1, 2019 · 12 comments
Closed

import_style=typescript with Bazel? #550

prestonvanloon opened this issue May 1, 2019 · 12 comments
Labels

Comments

@prestonvanloon
Copy link

It looks like only closure library generation is supported via Bazel

values = ["closure"],

Any plan to support this for typescript?

@stanley-cheung
Copy link
Collaborator

Previous discussion in #260

@Yannic
Copy link
Contributor

Yannic commented May 10, 2019

#260 was about supporting rules_closure with import_style=commonjs, while this seems to be about interoperability with rules_typescript. However, I feel like adding this comes with the same problems described in #260 because, IIRC, rules_typescript uses tsickle to have interoperability with rules_closure.
Always happy to talk if you have inputs towards finding a solution here.

@prestonvanloon
Copy link
Author

I'm not looking to use rules_closure, but use rules_typescript

@navono
Copy link

navono commented Jun 6, 2019

any update here?

@Globegitter
Copy link
Contributor

@Yannic would it it not be possible to use a parameterised aspect (see example https://docs.bazel.build/versions/master/skylark/aspects.html#aspect-definition-1) to let the user switch the aspect generation between import_style=closure/commonjs/commonjs+dts/typescript so we ever just have to generate aspects for one import style and therefore circumventing the issue? If I understand this correctly this should fix the issue for most users, as I imagine most users just care about having one of the import styles anyway. WDYT?

@Yannic
Copy link
Contributor

Yannic commented Jun 26, 2019

Generating the files really isn't the issue here, and I agree that most users probably won't run into the issue of having multiple import syles in their build-graph. But I don't think just allowing different import styles is going to solve anyone's problem.

Let me explain this a little more detailed:
The current implementation of closure_grpc_web_library is highly coupled with rules_closure, thus cannot handle anything typescript related (effectively ruling out import_style={typescript,commonjs+dts}). There is some support for commonjs in rules_closure, but there is absolutely no way of pulling in the matching protobuf/grpc-web runtimes from npm, which would leave users with completely broken, thus most likely unusable targets.
Additionally, I don't believe that anyone asking for alternative import styles is interested in using rules_closure, and, AFAICT, closure_grpc_web_library will not work with other JavaScript Bazel rules like rules_nodejs or rules_typescript.

In the end, I think we will shoot ourselves in the foot if we start supporting other import styles in closure_grpc_web_library.
What I'd be open to is creating something like commonjs_grpc_web_library that allows import_style={commonjs,commonjs+dts,typescript} and integrates with rules_nodejs and rules_typescript. Then, folks could choose whether they want to use import_style=closure and rules_closure or import_style={commonjs,commonjs+dts,typescript} and rules_{nodejs,typescript}.
Does that sound like it could solve your use-cases?

@Globegitter
Copy link
Contributor

Yep that sounds like a good option and would certainly fix the use-case I am interested in.

@nipun-sehrawat
Copy link

@Yannic - are you still planning to add commonjs_grpc_web_library? It'd be super helpful, thanks!

@Yannic
Copy link
Contributor

Yannic commented Oct 18, 2019

Yes, sorry for the delay.
I'm currently working on some general infrastructure around protobuf rules [1]. Once that work is done, I'll get back to this.

[1] https://github.com/bazelbuild/rules_proto

@CelsoSantos
Copy link

Well, I didn't want to be THAT guy, but I also need this and would very much like to have it. What is the progress on this and, is there any way I can help?

Yannic added a commit to Yannic/grpc-web that referenced this issue Mar 3, 2020
stanley-cheung pushed a commit that referenced this issue Mar 19, 2020
@johanbrandhorst
Copy link
Collaborator

There is support for this in rules_nodejs now: bazel-contrib/rules_nodejs#1695

loyalpartner pushed a commit to loyalpartner/grpc-web that referenced this issue Sep 4, 2020
@sampajano
Copy link
Collaborator

Hi :) Closing due to being obsolescence — closure_grpc_web_library was cleaned up #1138. Thanks for the discussion :)

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

No branches or pull requests

9 participants