fix: only return Observable types for NestJS if the option is switched on#823
fix: only return Observable types for NestJS if the option is switched on#823alexkb wants to merge 3 commits intostephenh:mainfrom
Conversation
|
Bump on this, using nestJS currently doesn't allow me to generate methods returning a promise |
|
@tmthecoder if you've got |
|
@alexkb I see the types be promise or observable for the controller implementations but the clients only have observable in the definitions, not sure why that is. I have the observable flag set to false manually in the proto gen command |
| findManyVillain(request: Observable<VillainById>): Observable<Villain>; | ||
|
|
||
| findManyVillainStreamIn(request: Observable<VillainById>): Promise<Villain> | Observable<Villain> | Villain; | ||
| findManyVillainStreamIn(request: Observable<VillainById>): Promise<Villain> | Villain; |
There was a problem hiding this comment.
@alexkb Disclaimer that I don't use NestJS, but wouldn't just Promise<Villain> make more sense here as a return type?
I agree that your PR is already better than the previous Observable<T> | Promise<T> | T, which imo seems just kind of maddening to use as a caller, b/c you'd have to constantly be checking what the return value was.
...ah, maybe this interface is meant for the server-side to implement, and we want to give flexibility to the server to return what it wants.
Are these interfaces used by both the client and the server? I don't remember...
There was a problem hiding this comment.
@stephenh NestJS can also have non async function for controllers and services hence returning T makes sense
this will valid implementation of function within nest framework
createPaymentLink(
request: CreatePaymentLinkRequest,
): CreatePaymentLinkResponse {
//...
return {
url: 'Payment at: https://lesgo.legolas.com',
}
}This wont be allowed if we go with Promise<T>
|
@alexkb shouldn't this also effect the client part which only returns the observable |
We're using
--ts_proto_opt=nestjs=trueand wanted to removeObservable's being returned, so we tried--ts_proto_opt=returnObservable=falsebut it had no effect.This PR fixes that issue so it does behave as expected. If it is incorrect, then I suggest you update the documentation to make it clear that
returnObservablehas no effect whennestjs=true. Thanks.