fix: update with PR feedback

README.markdown

@@ -446,11 +446,11 @@ Generated code will be placed in the Gradle build directory.
 
 - With `--ts_proto_opt=outputServices=false`, or `=none`, ts-proto will output NO service definitions.
 
-- With `--ts_proto_opt=outputBeforeRequest=true`, ts-proto will add a function definition to the Rpc interface definition with the signature: `beforeRequest(service: string, message: string, request: <RequestType>)`. It will will also automatically set `outputServices=default`. Each of the Service's methods will call `beforeRequest` before performing it's request.
+- With `--ts_proto_opt=rpcBeforeRequest=true`, ts-proto will add a function definition to the Rpc interface definition with the signature: `beforeRequest(service: string, message: string, request: <RequestType>)`. It will will also automatically set `outputServices=default`. Each of the Service's methods will call `beforeRequest` before performing it's request.
 
-- With `--ts_proto_opt=outputAfterResponse=true`, ts-proto will add a function definition to the Rpc interface definition with the signature: `afterResponse(service: string, message: string, response: <ResponseType>)`. It will will also automatically set `outputServices=default`. Each of the Service's methods will call `afterResponse` before returning the response.
+- With `--ts_proto_opt=rpcAfterResponse=true`, ts-proto will add a function definition to the Rpc interface definition with the signature: `afterResponse(service: string, message: string, response: <ResponseType>)`. It will will also automatically set `outputServices=default`. Each of the Service's methods will call `afterResponse` before returning the response.
 
-- With `--ts_proto_opt=outputErrorHandler=true`, ts-proto will add a function definition to the Rpc interface definition with the signature: `handleError(service: string, message: string, error: Error)`. It will will also automatically set `outputServices=default`.
+- With `--ts_proto_opt=rpcErrorHandler=true`, ts-proto will add a function definition to the Rpc interface definition with the signature: `handleError(service: string, message: string, error: Error)`. It will will also automatically set `outputServices=default`.
 
 - With `--ts_proto_opt=useAbortSignal=true`, the generated services will accept an `AbortSignal` to cancel RPC calls.
 

integration/before-after-request/parameters.txt

@@ -1 +1 @@
-outputBeforeRequest=true,outputAfterResponse=true,outputServices=default,outputServices=generic-definitions,
+rpcBeforeRequest=true,rpcAfterResponse=true,outputServices=default,outputServices=generic-definitions,

integration/handle-error-in-default-service/handle-error-test.ts

@@ -22,7 +22,7 @@ describe("before-after-request", () => {
     jest.clearAllMocks();
   });
 
-  it("performs handleError if error occurs during main request code block", async () => {
+  it("doesn't perform handleError if error occurs during encode step", async () => {
     const encodeSpy = jest.spyOn(GetBasicRequest, "encode").mockImplementation(() => {
       throw err;
     });
@@ -31,8 +31,8 @@ describe("before-after-request", () => {
     try {
       await client.GetBasic(req);
     } catch (error) {
-      expect(error).toBe(modifiedError);
-      expect(handleError).toHaveBeenCalledWith(BasicServiceServiceName, "GetBasic", err);
+      expect(error).toBe(err);
+      expect(handleError).not.toHaveBeenCalled();
     }
     encodeSpy.mockRestore();
   });

integration/handle-error-in-default-service/parameters.txt

@@ -1 +1 @@
-outputServices=default,outputErrorHandler=true
+outputServices=default,rpcErrorHandler=true

integration/handle-error-in-default-service/simple.ts

@@ -139,25 +139,20 @@ export class BasicServiceClientImpl implements BasicService {
     this.GetBasic = this.GetBasic.bind(this);
   }
   GetBasic(request: GetBasicRequest): Promise<GetBasicResponse> {
-    try {
-      const data = GetBasicRequest.encode(request).finish();
-      const promise = this.rpc.request(this.service, "GetBasic", data);
-      return promise.then((data) => {
-        try {
-          return GetBasicResponse.decode(_m0.Reader.create(data));
-        } catch (error) {
-          if (error instanceof Error && this.rpc.handleError) {
-            throw this.rpc.handleError(this.service, "GetBasic", error);
-          }
-          throw error;
-        }
-      });
-    } catch (error) {
+    const data = GetBasicRequest.encode(request).finish();
+    const promise = this.rpc.request(this.service, "GetBasic", data);
+    return promise.then((data) => {
+      try {
+        return GetBasicResponse.decode(_m0.Reader.create(data));
+      } catch (error) {
+        return Promise.reject(error);
+      }
+    }).catch((error) => {
       if (error instanceof Error && this.rpc.handleError) {
-        throw this.rpc.handleError(this.service, "GetBasic", error);
+        return Promise.reject(this.rpc.handleError(this.service, "GetBasic", error));
       }
-      throw error;
-    }
+      return Promise.reject(error);
+    });
   }
 }
 

integration/handle-error-with-after-response/handle-error-test.ts

@@ -31,8 +31,8 @@ describe("before-after-request", () => {
     try {
       await client.GetBasic(req);
     } catch (error) {
-      expect(error).toBe(modifiedError);
-      expect(handleError).toHaveBeenCalledWith(BasicServiceServiceName, "GetBasic", err);
+      expect(error).toBe(err);
+      expect(handleError).not.toHaveBeenCalled();
     }
     encodeSpy.mockRestore();
   });

integration/handle-error-with-after-response/parameters.txt

@@ -1 +1 @@
-outputServices=default,outputAfterResponse=true,outputErrorHandler=true
+outputServices=default,rpcAfterResponse=true,rpcErrorHandler=true

integration/handle-error-with-after-response/simple.ts

@@ -139,29 +139,24 @@ export class BasicServiceClientImpl implements BasicService {
     this.GetBasic = this.GetBasic.bind(this);
   }
   GetBasic(request: GetBasicRequest): Promise<GetBasicResponse> {
-    try {
-      const data = GetBasicRequest.encode(request).finish();
-      const promise = this.rpc.request(this.service, "GetBasic", data);
-      return promise.then((data) => {
-        try {
-          const response = GetBasicResponse.decode(_m0.Reader.create(data));
-          if (this.rpc.afterResponse) {
-            this.rpc.afterResponse(this.service, "GetBasic", response);
-          }
-          return response;
-        } catch (error) {
-          if (error instanceof Error && this.rpc.handleError) {
-            throw this.rpc.handleError(this.service, "GetBasic", error);
-          }
-          throw error;
+    const data = GetBasicRequest.encode(request).finish();
+    const promise = this.rpc.request(this.service, "GetBasic", data);
+    return promise.then((data) => {
+      try {
+        const response = GetBasicResponse.decode(_m0.Reader.create(data));
+        if (this.rpc.afterResponse) {
+          this.rpc.afterResponse(this.service, "GetBasic", response);
         }
-      });
-    } catch (error) {
+        return response;
+      } catch (error) {
+        return Promise.reject(error);
+      }
+    }).catch((error) => {
       if (error instanceof Error && this.rpc.handleError) {
-        throw this.rpc.handleError(this.service, "GetBasic", error);
+        return Promise.reject(this.rpc.handleError(this.service, "GetBasic", error));
       }
-      throw error;
-    }
+      return Promise.reject(error);
+    });
   }
 }
 

src/generate-services.ts

@@ -121,25 +121,25 @@ function generateRegularRpcMethod(ctx: Context, methodDesc: MethodDescriptorProt
   const maybeAbortSignal = options.useAbortSignal ? "abortSignal || undefined," : "";
 
   let errorHandler;
-  if (options.outputErrorHandler) {
+  if (options.rpcErrorHandler) {
     errorHandler = code`
       if (error instanceof Error && this.rpc.handleError) {
-        throw this.rpc.handleError(this.service, "${methodDesc.name}", error);
+        return Promise.reject(this.rpc.handleError(this.service, "${methodDesc.name}", error));
       }
-      throw error;
+      return Promise.reject(error);
     `;
   }
 
   let encode = code`${rawInputType}.encode(request).finish()`;
   let beforeRequest;
-  if (options.outputBeforeRequest) {
+  if (options.rpcBeforeRequest) {
     beforeRequest = code`
     if (this.rpc.beforeRequest) {
       this.rpc.beforeRequest(this.service, "${methodDesc.name}", request);
     }`;
   }
   let decode = code`${rawOutputType}.decode(${Reader}.create(data))`;
-  if (options.outputAfterResponse) {
+  if (options.rpcAfterResponse) {
     decode = code`
       const response = ${rawOutputType}.decode(${Reader}.create(data));
       if (this.rpc.afterResponse) {
@@ -184,7 +184,6 @@ function generateRegularRpcMethod(ctx: Context, methodDesc: MethodDescriptorProt
     ${methodDesc.formattedName}(
       ${joinCode(params, { on: "," })}
     ): ${responsePromiseOrObservable(ctx, methodDesc)} {
-      ${options.outputErrorHandler ? "try {" : ""}
       const data = ${encode}; ${beforeRequest ? beforeRequest : ""}
       const ${returnVariable} = this.rpc.${rpcMethod}(
         ${maybeCtx}
@@ -194,7 +193,6 @@ function generateRegularRpcMethod(ctx: Context, methodDesc: MethodDescriptorProt
         ${maybeAbortSignal}
       );
       return ${returnStatement};
-      ${errorHandler ? code` } catch (error) { ${errorHandler} }` : ""}
     }
   `;
 }
@@ -215,11 +213,16 @@ function createDefaultServiceReturn(
     }
   }
 
-  if (options.outputAfterResponse && errorHandler) {
-    return code`promise.then(data => { ${tryCatchBlock(decode, errorHandler)} })`;
-  } else if (errorHandler) {
-    return code`promise.then(data => { ${tryCatchBlock(code`return ${decode}`, errorHandler)} } )`;
-  } else if (options.outputAfterResponse) {
+  if (errorHandler) {
+    let tryBlock = decode;
+    if (!options.rpcAfterResponse) {
+      tryBlock = code`return ${decode}`;
+    }
+    return code`promise.then(data => { ${tryCatchBlock(
+      tryBlock,
+      code`return Promise.reject(error);`,
+    )}}).catch((error) => { ${errorHandler} })`;
+  } else if (options.rpcAfterResponse) {
     return code`promise.then(data => { ${decode} } )`;
   }
   return code`promise.then(data => ${decode})`;
@@ -404,17 +407,17 @@ export function generateRpcType(ctx: Context, hasStreamingMethods: boolean): Cod
   const maybeAbortSignalParam = options.useAbortSignal ? "abortSignal?: AbortSignal," : "";
   const methods = [[code`request`, code`Uint8Array`, code`Promise<Uint8Array>`]];
   const additionalMethods = [];
-  if (options.outputBeforeRequest) {
+  if (options.rpcBeforeRequest) {
     additionalMethods.push(
       code`beforeRequest?<T extends { [k in keyof T]: unknown }>(service: string, method: string, request: T): void;`,
     );
   }
-  if (options.outputAfterResponse) {
+  if (options.rpcAfterResponse) {
     additionalMethods.push(
       code`afterResponse?<T extends { [k in keyof T]: unknown }>(service: string, method: string, response: T): void;`,
     );
   }
-  if (options.outputErrorHandler) {
+  if (options.rpcErrorHandler) {
     additionalMethods.push(code`handleError?(service: string, method: string, error: Error): Error;`);
   }
 

src/options.ts

@@ -87,9 +87,9 @@ export type Options = {
   outputExtensions: boolean;
   outputIndex: boolean;
   M: { [from: string]: string };
-  outputBeforeRequest: boolean;
-  outputAfterResponse: boolean;
-  outputErrorHandler: boolean;
+  rpcBeforeRequest: boolean;
+  rpcAfterResponse: boolean;
+  rpcErrorHandler: boolean;
 };
 
 export function defaultOptions(): Options {
@@ -146,9 +146,9 @@ export function defaultOptions(): Options {
     outputExtensions: false,
     outputIndex: false,
     M: {},
-    outputBeforeRequest: false,
-    outputAfterResponse: false,
-    outputErrorHandler: false,
+    rpcBeforeRequest: false,
+    rpcAfterResponse: false,
+    rpcErrorHandler: false,
   };
 }
 
@@ -246,7 +246,7 @@ export function optionsFromParameter(parameter: string | undefined): Options {
     options.exportCommonSymbols = false;
   }
 
-  if (options.outputBeforeRequest || options.outputAfterResponse || options.outputErrorHandler) {
+  if (options.rpcBeforeRequest || options.rpcAfterResponse || options.rpcErrorHandler) {
     const includesGeneric = options.outputServices.includes(ServiceOption.GENERIC);
     options.outputServices = [ServiceOption.DEFAULT];
     if (includesGeneric) {

tests/options-test.ts

@@ -26,11 +26,11 @@ describe("options", () => {
         "nestJs": true,
         "oneof": "properties",
         "onlyTypes": false,
-        "outputAfterResponse": false,
-        "outputBeforeRequest": false,
+        "rpcAfterResponse": false,
+        "rpcBeforeRequest": false,
         "outputClientImpl": false,
         "outputEncodeMethods": false,
-        "outputErrorHandler": false,
+        "rpcErrorHandler": false,
         "outputExtensions": false,
         "outputIndex": false,
         "outputJsonMethods": true,
@@ -168,28 +168,28 @@ describe("options", () => {
     });
   });
 
-  it("outputAfterResponse implies default service", () => {
-    const options = optionsFromParameter("outputAfterResponse=true");
+  it("rpcAfterResponse implies default service", () => {
+    const options = optionsFromParameter("rpcAfterResponse=true");
     expect(options).toMatchObject({
-      outputAfterResponse: true,
+      rpcAfterResponse: true,
       outputServices: [ServiceOption.DEFAULT],
     });
   });
 
-  it("outputBeforeRequest implies default service", () => {
-    const options = optionsFromParameter("outputBeforeRequest=true");
+  it("rpcBeforeRequest implies default service", () => {
+    const options = optionsFromParameter("rpcBeforeRequest=true");
     expect(options).toMatchObject({
-      outputBeforeRequest: true,
+      rpcBeforeRequest: true,
       outputServices: [ServiceOption.DEFAULT],
     });
   });
 
-  it("outputAfterResponse implies default service but allows generics too", () => {
+  it("rpcAfterResponse implies default service but allows generics too", () => {
     const options = optionsFromParameter(
-      "outputBeforeRequest=true,outputServices=generic-definitions,outputServices=default",
+      "rpcBeforeRequest=true,outputServices=generic-definitions,outputServices=default",
     );
     expect(options).toMatchObject({
-      outputBeforeRequest: true,
+      rpcBeforeRequest: true,
       outputServices: [ServiceOption.DEFAULT, ServiceOption.GENERIC],
     });
   });