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

Generate both grpc-node and grpc-web simultaneously #211

Closed
Multiply opened this issue Nov 5, 2019 · 11 comments
Closed

Generate both grpc-node and grpc-web simultaneously #211

Multiply opened this issue Nov 5, 2019 · 11 comments
Labels

Comments

@Multiply
Copy link

Multiply commented Nov 5, 2019

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?

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 6, 2019 via email

@Multiply
Copy link
Author

Multiply commented Nov 6, 2019

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.

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 6, 2019 via email

@Multiply
Copy link
Author

Multiply commented Nov 6, 2019

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.

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.

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 6, 2019 via email

@Multiply
Copy link
Author

Multiply commented Nov 6, 2019

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 service parameter, we won't be backwards compatible, if we were to wrap the base typescript files in an if on the same parameter.

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 6, 2019 via email

@Multiply
Copy link
Author

Multiply commented Nov 6, 2019

@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.

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 6, 2019 via email

@Multiply
Copy link
Author

Multiply commented Nov 6, 2019

@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.

@stale
Copy link

stale bot commented Feb 4, 2020

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.

@stale stale bot added the wontfix label Feb 4, 2020
@stale stale bot closed this as completed Feb 11, 2020
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

2 participants