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

Lowercasing does not lowercase "API" of FindAPIKeys, for example. #698

Closed
iangregsondev opened this issue Nov 8, 2022 · 6 comments
Closed

Comments

@iangregsondev
Copy link
Contributor

Hi,

I have a GRPC endpoint like so

 rpc FindAPIKeys(FindAPIKeysRequest) returns (FindAPIKeysResponse) {}

The type that is creates is the following (notice the upper case API)

  findAPIKeys(request: FindAPIKeysRequest, metadata?: Metadata): Observable<FindAPIKeysResponse>;

This actually fails because of the grpc Options are set to lowercase see https://github.com/grpc/grpc-node/blob/master/packages/proto-loader/README.md

The valid types is (if I change it manually it works :-) )

  findApiKeys(request: FindAPIKeysRequest, metadata?: Metadata): Observable<FindAPIKeysResponse>;

Anybody know if there is a way around this ?

Thanks in advance

Ian

@iangregsondev
Copy link
Contributor Author

iangregsondev commented Nov 9, 2022

@stephenh Would you like me to look at putting in a pull request?

p.s. Even though I am using it through Nestjs, this issue also applies to non nestjs generations.

Actually, in my current case, I am using it through nestJS but its not a nestjs grpc created service, so I am actually using the following - notice no nestjs being passed in , reason being is that if I choose nestjs it creates the controller - which in this case I don't need.

protoc --plugin=./node_modules/.bin/protoc-gen-ts_proto --ts_proto_opt=onlyTypes=true,returnObservable=true,addGrpcMetadata=true,lowerCaseServiceMethods=true,useOptionals=all --ts_proto_out=. ./proto/**/*.proto

Cheers

@iangregsondev
Copy link
Contributor Author

I would like to know if the change i make would break anything, I doubt it, as this is controlled on the grpc options end.

I am just assuming nobody has grpc endpoints(proto) like the one mentioned above, which is why it's not been mentioned previously.

@stephenh
Copy link
Owner

Hey @iangregsondev ! Sorry for the late reply, just catching up...

Makes sense, "does camel case mean 'API' or 'Api'?" is a common disagreement between different camelCase implementations.

If you wanted to submit a PR to fix this, that'd be great. If I'm following, it would be around here:

https://github.com/stephenh/ts-proto/blob/main/src/utils.ts#L166

To your point of it being a breaking change, I think the camelCase method is used for quite a few things in ts-proto (other than just the GRPC method names), so maybe we could make a new/dedicated camelCaseGrpc function that mimics their Api-vs-API output, and use that specifically/only for that formatName method?

Thanks!

@iangregsondev
Copy link
Contributor Author

Hi @stephenh , sorry for the long delay; busy with other work stuff. Going to take this, this morning/afternoon. Looks pretty easy to fix and I will get over a PR.

Cheers.

@iangregsondev
Copy link
Contributor Author

@stephenh Initial PR made, let me know your thoughts over there.

#721

There is a failing check, not sure what that is.

Thanks!

@iangregsondev
Copy link
Contributor Author

Thanks, @stephenh all tested my end, and it seems to work :-)

Thanks again!

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