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 disable_servicetype option? #205

Closed
ColinLaws opened this issue Jan 19, 2022 · 8 comments
Closed

Add disable_servicetype option? #205

ColinLaws opened this issue Jan 19, 2022 · 8 comments

Comments

@ColinLaws
Copy link
Contributor

I recently upgraded to the latest version of the @protobuf-ts/* packages and updated the option names passed to protoc to their new ones, for instance disable_service_client was renamed to force_client_none; however, I am no longer able to prevent protobuf-ts from generating services in my typescript files.

I have tried to dig into the source code for this package and understand how options for service generation are used, but I can't seem to find anything that is standing out. I have also tried to remove all other options I was using and only specified force_client_none but to no avail. I have also tried using the client_none option, but again, services are still generated.

I was wondering if anyone else was experiencing this issue, or if anyone else could try to reproduce this.

The full command I am running is:

npx protoc \
    --proto_path="${MY_PATH}" \
    --plugin="protoc-gen-ts=${PROTOC_PATH}" \
    --ts_out="${PROTOC_OUT_DIR}" \
    --ts_opt force_client_none \
    --ts_opt generate_dependencies \
    --ts_opt long_type_string \
    --experimental_allow_proto3_optional \
   ${FILES}

While my protos are generated fine, again, the services are being generated and this is problematic for the project I am working on.

@timostamm
Copy link
Owner

Sorry for the inconvenience. Version 2.0.0 had a few intentional breaking changes (following semver):
https:/timostamm/protobuf-ts/blob/master/CHANGELOG.md#v200

force_client_none not working must be a bug that slipped in in v2.2.0. The options parsing was refactored and unfortunately, it doesn't have test coverage.

@timostamm
Copy link
Owner

I gave it a look, but force_client_none still works. 2.0.0 added generation of service metadata:

/**
 * @generated ServiceType for protobuf service spec.ExampleService
 */
export const ExampleService = new ServiceType("spec.ExampleService", [
    { name: "Unary", options: {}, I: ExampleRequest, O: ExampleResponse },
    { name: "ServerStream", serverStreaming: true, options: {}, I: ExampleRequest, O: ExampleResponse },
    { name: "ClientStream", clientStreaming: true, options: {}, I: ExampleRequest, O: ExampleResponse },
    { name: "Bidi", serverStreaming: true, clientStreaming: true, options: {}, I: ExampleRequest, O: ExampleResponse }
]);

Do you mean those?

@ColinLaws
Copy link
Contributor Author

Ah yes, I see, I am referring to the metadata. In my haste I hadn’t examined exactly what the symbol was referring to. I just moved from 1.0.13 to 2.2.1 and updated my proto generator script that uses this plugin, and concluded the appearance of the metadata meant something went wrong with my migration to the new plugin.

Currently this is making it a pain in my use case, since I export my generated models and services to a public api surface in an Angular library, and the names of the service metadata constants and my services are conflicting because they follow the same naming convention. To get around it, I’d have to export my generated typescript models by hand to avoid the conflict.

I suggest a change to append the word Metadata on the end of it.

@timostamm
Copy link
Owner

Got it, that's a good point. If we appended Metadata, it might break existing code for other users.

I wonder if it would have been better to only generate the service metadata if the user opts in with a plugin option - and automatically enable it if the user sets an option that requires it. However, that too is a breaking change, so it would probably be best to do that in v3.

In the meantime, what do you think about an option to disable the generation explicitly? The downside is that it would really only be a temporary option, and be removed in v3.

@ColinLaws
Copy link
Contributor Author

I agree, definitely keep it generating the metadata for services at the moment to avoid breaking dependents of the package. I would also agree that adding an option to prevent the metadata from generating would be a good work around for anyone who may potentially run into the issue I am.

With that said, I would be willing to make this change, however I am not very familiar with the intricacies of this package. I can take a look later today at what would need to be added, but without having to do too much guess work, could you point me in the right direction for where the metadata is being generated? From my understanding of how the project is laid out, the @protobuf-ts/plugin package is the actual implementation, and the @protobuf-ts/plugin-framework is a set of utilities you separated which help in building protoc plugins.

Thanks for your help on this, I really appreciate it. This package is a huge help to something we're building.

@timostamm
Copy link
Owner

That would be much appreciated.

I think the new option can simply disable the following line with an if-statement:

genServiceType.generateServiceType(outMain, descriptor)

The new option should be added somewhere here:

And then be parsed into the internal options object (which needs a new property) here:

export function makeInternalOptions(

Then it will be available via options.xxx for the if-statement.

The new option probably belongs into the "misc" category (since it is independent of client and server). "disable_servicetype" seems like a fitting name.

@timostamm timostamm changed the title force_client_none option is not working Add disable_servicetype option? Feb 21, 2022
@ColinLaws
Copy link
Contributor Author

Sorry for the wait! I haven't had much time and got side tracked. I hope to get these changes in soon. Your notes really helped me jump in and get started. Also, installing/testing/building was super easy with the Makefile.

@ColinLaws
Copy link
Contributor Author

Submitted PR #268 to add this functionality.

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

No branches or pull requests

2 participants