-
Notifications
You must be signed in to change notification settings - Fork 172
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
Generate both grpc-node and grpc-web simultaneously #211
Comments
Do you have this use case?
…On Tue, 5 Nov 2019, 22:15 Jens Ulrich Hjuler Fosgerau, < ***@***.***> wrote:
Would it make sense to be able to generate both grpc-node and grpc-web
services at the same time, rather than the current if/else we have today?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#211?email_source=notifications&email_token=AABQ36N7QFT3YCJZE5HFBDLQSHWBDA5CNFSM4JJLBNX2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HXBKJHA>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQ36NMIE3KQ5VJ2YBMLWLQSHWBDANCNFSM4JJLBNXQ>
.
|
I would like to use the service interface from grpc-node to build the GRPC Server, and would like to use the client from grpc-node to implement server-side clients. And in turn use grpc-web client to implement clients that run in the browser. Alternatively, I would like to optionally generate either the base d.ts files, or one of the services, to only generate a single set of files in one protoc call, or all of them in one go. |
That makes sense.
Do you have thoughts on how this could work from a user perspective?
…On Wed, 6 Nov 2019 at 07:46, Jens Ulrich Hjuler Fosgerau < ***@***.***> wrote:
I would like to use the service interface from grpc-node to build the GRPC
Server, and would like to use the client from grpc-node to implement
server-side clients.
And in turn use grpc-web client to implement clients that run in the
browser.
Alternatively, I would like to optionally generate either the base d.ts
files, or one of the services, to only generate a single set of files in
one protoc call, or all of them in one go.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#211?email_source=notifications&email_token=AABQ36J6TDR6DXYMZSVY3KDQSJY4XA5CNFSM4JJLBNX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDFTEUQ#issuecomment-550187602>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQ36OUTX3RQ4ELJRG5ZXTQSJY4XANCNFSM4JJLBNXQ>
.
|
Scenario 1: Generate everything in one call Scenario 2: Generate only one set of files per call The latter proposal can be backwards compatible, and will support both scenarios. All in one go, or one set of files at a time. Edit: I don't know the codebase too well, so this might not be how any of this is intended to work at all. Just an idea I had. |
Awesome, thank you for articulating these so clearly :)
Do you have a preference for either option? My personal preference would be
for the first option, but I'm open to persuasion.
The latter proposal can be backwards compatible, and will support both
scenarios. All in one go, or one set of files at a time.
Do you think that Option 1 is not backwards compatible?
…On Wed, 6 Nov 2019 at 10:40, Jens Ulrich Hjuler Fosgerau < ***@***.***> wrote:
*Scenario 1: Generate everything in one call*
Change the service parameter to allow comma separated values:
https://github.com/improbable-eng/ts-protoc-gen/blob/master/src/index.ts#L30-L31
Change the else if to an if:
https://github.com/improbable-eng/ts-protoc-gen/blob/master/src/index.ts#L49-L55
*Scenario 2: Generate only one set of files per call*
Add another parameter generate (or some other meaningful name that is not
service), and allow comma separated values like in the proposal above.
Add an if around the base typescript files:
https://github.com/improbable-eng/ts-protoc-gen/blob/master/src/index.ts#L44-L47
When generate parameter contains base, output the base typescript files
we just added an if around.
When generate parameter contains grpc-web, output the service web
typescript files.
When generate parameter contains grpc-node, output the service node
typescript files.
------------------------------
The latter proposal can be backwards compatible, and will support both
scenarios. All in one go, or one set of files at a time.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#211?email_source=notifications&email_token=AABQ36OBXFFVUGH2CGNL2GTQSKNIVA5CNFSM4JJLBNX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDGC4YY#issuecomment-550252131>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQ36MZG5Y4G5NBKTIBGYDQSKNIVANCNFSM4JJLBNXQ>
.
|
Scenario 1 is definitely backwards compatible, but it does not allow you to skip the generation of the base fileset. So if I were to run two protoc on the same directory, one for grpc-web, and one for grpc-node, it would cause the base typescript files to be generated twice, unless we go for an approach like Scenario 2. Edit: Just to clarify my initial backwards compatible comment; It was meant as if we reuse the |
Hey Jens,
Scenario 1 is definitely backwards compatible, but it does not allow you
to skip the generation of the base fileset. So if I were to run two protoc
on the same directory, one for grpc-web, and one for grpc-node, it would
cause the base typescript files to be generated twice,
But if you are able to specify multiple 'service' outputs (ie:
`service=grpc-web,grpc-node') then there would be no need to run protoc
twice? Am I missing something?
…On Wed, 6 Nov 2019 at 11:49, Jens Ulrich Hjuler Fosgerau < ***@***.***> wrote:
Scenario 1 is definitely backwards compatible, but it does not allow you
to skip the generation of the base fileset. So if I were to run two protoc
on the same directory, one for grpc-web, and one for grpc-node, it would
cause the base typescript files to be generated twice, unless we go for an
approach like Scenario 2
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#211?email_source=notifications&email_token=AABQ36MWL34YJKG7XJ3DIY3QSKVL3A5CNFSM4JJLBNX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDGISSA#issuecomment-550275400>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQ36JUYKSRU3OLBLJ7KK3QSKVL3ANCNFSM4JJLBNXQ>
.
|
@jonnyreeves that's correct. The 'need' for Scenario 2 stems from Dig-Doug/rules_typescript_proto#10 (comment) where I'd like to avoid generating the base ts files twice. I can work around it, though. Was just simple to implement (in a backwards-compatible way) now that I'm already looking at implementing one of the scenarios above. Let me know what you think, and I can try to submit a PR tomorrow. |
Ah great, thanks for sharing.
PRs welcome, happy for you to take the lead on the design. Let me know if I
can help 👍
…On Wed, 6 Nov 2019, 19:01 Jens Ulrich Hjuler Fosgerau, < ***@***.***> wrote:
@jonnyreeves <https://github.com/jonnyreeves> that's correct.
The 'need' for Scenario 2 stems from Dig-Doug/rules_typescript_proto#10
(comment)
<Dig-Doug/rules_typescript_proto#10 (comment)>
where I'd like to avoid generating the base ts files twice.
I can work around it, though. Was just simple to implement (in a
backwards-compatible way) now that I'm already look at implementing one of
the scenarios above.
Let me know what you think, and I can try to submit a PR tomorrow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#211?email_source=notifications&email_token=AABQ36M24TSH5KQP5R4GRN3QSMICJA5CNFSM4JJLBNX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDHUCEY#issuecomment-550453523>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQ36IVM6R3VDFKPC4DGD3QSMICJANCNFSM4JJLBNXQ>
.
|
@jonnyreeves I ended up going with scenario 2. I've made a WIP PR here: #212 Please let me know of any changes or missing steps I need to complete, for it to be in a mergeable state. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Would it make sense to be able to generate both grpc-node and grpc-web services at the same time, rather than the current if/else we have today?
The text was updated successfully, but these errors were encountered: