fix: conflicting option with nestJs and outputIndex #1018
Conversation
- when we have nestJs=true and outputIndex=true, multiple protobufPackage variables will be generated, which breaks the barrel file type checking, since we have multiple exports with the same name. - the default nestjs distinct PACKAGE_NAME and SERVICE_NAME variables won't be generated.
| @@ -132,7 +132,8 @@ export function generateFile(ctx: Context, fileDesc: FileDescriptorProto): [stri | |||
| const chunks: Code[] = []; | |||
|
|
|||
| // Indicate this file's source protobuf package for reflective use with google.protobuf.Any | |||
| if (options.exportCommonSymbols) { | |||
| // since when nestJs=true, we have distinct protobuf package names we should exclude it here | |||
| if (options.exportCommonSymbols && !options.nestJs) { | |||
There was a problem hiding this comment.
Hm, I wonder if we have to do it this way...
I think I would prefer keeping protobufPackage considered a "common symbol", and not give this specific nestJs handling.
And imo outputIndex should probably keep turning off exportCommonSymbols, even in nestJs, because the "common symbols" are usually ts-proto helper methods that would be defined in each file.
It seems like your core issue is that you'd like to revert:
https://github.com/stephenh/ts-proto/pull/916/files
Because your assertion is that, for you, the FOO_PACKAGE_NAME files are actually unique per file, and you want them in the barrel files.
My guess is that @eladhaim 's NestJS projects don't have this condition, so this ends up being a project-by-project assumption.
If so, I don't think we can know the right thing to do by just tweaking options.exportCommonSymbols & options.nestJs to try and guess your individual-but-opposite use cases.
So we'll probably need a new flag, like exportNestJsConsts that is usually true, but @eladhaim could then disable.
That way @TheMichio could leave exportCommonSymbols=false
There was a problem hiding this comment.
Hi @stephenh 馃憢
so sorry for late reply, been busy past couple of weeks.
I totally understand your point and you're right, it depends on the project, I can create the flag and set it up like you mentioned. will do it in a week or two and will let you know. see if you like it.
thanks thanks 馃槈
Changes:
file type checking, since we have multiple exports with the same name.
@stephenh there is a conflict of options and behavior when we have
nestJs=trueandoutputIndex=trueat the same time, in whichoutputIndexoverrides the defaultPACKAGE_NAMEandSERVICE_NAMEgeneration ofnestjsoption.dropping these two variables is a huge loss IMO, since we use them as injection tokens in services and in module initializations.
please let me know what you think, thanks thanks 馃檹
Note:
the changes on generated tests are needed TBH, current generated codes are counter-productive in actual Nestjs GRPC Microservices setup.