Add module param to bazel rules for specifying amd or commonjs

[kellycampbell]

Changes

• add option to bazel typescript_proto_library to leave the generated code in commonjs format for nodejs use

Verification

• Example project at https://github.com/kellycampbell/example-bazel-proto-ts

[kellycampbell]

Fixes #177

[jonny-improbable]

@Dig-Doug, could you please take a look?

[Dig-Doug]

@kellycampbell, thanks for the PR.

I originally considered this approach when I was implementing the logic for this rule. As I was looking through the existing JS proto library implementations, I came across this comment. It explains why having a user-defined import style is problematic in some cases.

After looking at #177, I agree with @mattem, it would be better if the output type could be requested. Currently ES6 code is not being generated, have you tried updating the rule to generate it separately? Would that work for your use case?

[kellycampbell]

@Dig-Doug thanks. That makes sense. My use-case is server-side nodejs. I think the es6 method would work for that. @mattem do you already have something in progress for converting the protoc output files to es6?

[eordano]

Hi there,

First of all, thank you very much for maintaining this library. I'm using bazel 0.28 and found @Dig-Doug's work very helpful (and recent)

Secondly, I feel that even though allowing users to specify the module mechanism might be problematic -- it's worse than the alternative (will have to fork).

[eordano]

(I'm looking for es6 imports -- amd doesn't work for us)

[Dig-Doug]

Hi @eordano, do you have some more info about the problem your having?

I had a need for es6 too and made some changes in #185. Have you tried those? If you're still having issues with the standard TS / JS bazel toolchain, we should fix them.

P.S. there is a pending PR for compatibility with 0.28 :)

[eordano]

Found #185 later thank you!

…

On Tue, 30 Jul 2019 at 03:17 Dig-Doug ***@***.***> wrote: Hi @eordano <https://github.com/eordano>, do you have some more info about the problem your having? I had a need for es6 too and made some changes in #185 <#185>. Have you tried those? If you're still having issues with the standard TS / JS bazel toolchain, we should fix them. P.S. there is a pending PR for compatibility with 0.28 :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#179?email_source=notifications&email_token=AAAKN7XJ475ZXB6IAXN4I33QB7MFZA5CNFSM4HP6PUBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3C4YYA#issuecomment-516279392>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKN7XCIKFHCABROM4KIO3QB7MFZANCNFSM4HP6PUBA> .

[jonny-improbable]

@kellycampbell, @Dig-Doug what are the suggested next steps for this PR? It's currently in conflict so it needs rebasing at the very least.

[Dig-Doug]

I actually tried using NodeJS yesterday myself and found a few issues :) -- I've submitted #194 to fix them.

@kellycampbell - Can you try out the changes in #194 and see if it works for you?

[jonny-improbable]

@Dig-Doug is this pull now obsolete given #194 has been merged?

[Dig-Doug]

Yes, I believe so. I've been using the TS protos in nodejs successfully.

[eordano]

Can also confirm it's working 👍

…

On Thu, 15 Aug 2019 at 15:12 Dig-Doug ***@***.***> wrote: Yes, I believe so. I've been using the TS protos in nodejs successfully. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#179?email_source=notifications&email_token=AAAKN7UZFI4IXMAS7HK2QUDQEVI4VA5CNFSM4HP6PUBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4LYPDI#issuecomment-521635725>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKN7SCJQWDFUHAMOV2GRDQEVI4VANCNFSM4HP6PUBA> .