-
Notifications
You must be signed in to change notification settings - Fork 343
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
better nestjs support #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few super minor comments but really looks great. @toonvanstrijp let me know once/if you want to address any of my nudges and I'll go ahead and merge it.
file = file.addFunction(generateNestjsGrpcServiceMethodsDecorator(serviceDesc, options)); | ||
|
||
let serviceConstName = `${camelToSnake(serviceDesc.name)}_NAME`; | ||
if(!serviceDesc.name.toLowerCase().endsWith('service')){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite following this? Would it hurt to just always export the const? Even if the user, should their service end with service
doesn't technically need it, it's there for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I quite like that, @stephenh i think its explained here
Note: Based on the
.proto
we'll generate aconst
for exampleHERO_PACKAGE_NAME
andHERO_SERVICE_NAME
this way your code breaks if you change your package or service name. (It's safer to have compiler errors than runtime errors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenh This way we offer more consistency in the naming. If the user happened to have a service inside their .proto
named: hero
then it would end up as HERO_NAME
. We could do it without forcing the SERVICE
in here but this also makes me think. If we go for this approach we also should force the service
in the controller
and client
interface name?
@stephenh, @iangregsondev let me know what you guys think is a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay.
If we go for this approach we also should force the service in the
controller and client interface name?
Eh, I could go either way. I think what you have now is fine (i.e. don't force Service
into the interface names).
Most users probably do have Service
as a suffix of their service names anyway? Or maybe they prefer something like HeroRpc
(just making something up, but some other suffix that is "like service") so by assuming "oh you must want Service
suffixed", we'd be doing the wrong thing.
Better to make less assumptions I think.
In that vein, I'm tempted to nudge the "does name end in service" check here out to be consistent, i.e. maybe they want would HERO_RPC_NAME
🤷 .
(I agree having service Hero
is kind of odd, so I'm assuming most users won't do that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either HeroRpc
or HeroService
look fine to me but I prefer HeroService
since it's a service
inside the proto.
Also I think we should no force the user to much as you said.
So we can resolve this issue and leave it as it is now?
@stephenh I fixed some of the changes you requested, and commented on the changes that need more info. |
@stephenh @iangregsondev please review this also: 93a4b28 |
This pull request is discussed in #59, please see this issue for more context.
Let me know if anything needs to be changed. This pull request will be a breaking change, since we changed the output when the
nestJs=true
option is set.I've changed/improved the following:
nestjs
section to thereadme
file: https://github.com/debugged-software/ts-proto#nestjsnestjs=true
)nestjs
.nestjs
. (generateNestjsService
andgenerateNestjsServiceClient
).nestjs=true
it also generates anclass decorator
for exampleHeroServiceControllerMethods
(based on the name of the service). Normally innestjs
you would need to add@GrpcMethod('SERVICE_NAME')
or@GrpcStreamMethod('SERVICE_NAME')
. This however would fail in runtime when you accidentally change your service name. But because we generate thisclass decorator
for you, you won't have this risk anymore. The only thing you need to do is provide thisdecorator
on thecontroller
class. The decorator will take care of applying the@GrpcMethod
and@GrpcStreamMethod
.proto
we'll generate aconst
for exampleHERO_PACKAGE_NAME
andHERO_SERVICE_NAME
this way your code breaks if you change your package or service name. (It's safer to have compiler errors than runtime errors)