Add nullable to RpcError of generated typescript code

[arluko]

I am working with an angular application and eslint. When I enable the @typescript-eslint/no-unnecessary-condition rule (see https://typescript-eslint.io/rules/no-unnecessary-condition) it triggers a linting error which is cause by the generated code from the grpc_generator.cc file.

Excerpt of the generated code:

  setFoo(
    request: xyz_api_pb.FooRequest,
    metadata: grpcWeb.Metadata | null,
    callback?: (
      err: grpcWeb.RpcError, // => err is not typed as `grpcWeb.RpcError | null`
      response: xyz_api_pb.FooResponse) => void
  ) {
    // ...
  }

Example implementation of the service with angular:

  public foo(): Promise<void> {
    const request = new FooRequest();

    return new Promise((resolve, reject) => {
      this._apiClient.setFoo(request, {}, (err, _response) => {
        if (err) { // => Unnecessary conditional, value is always truthy 
          reject(err);
        } else {
          resolve();
        }
      });
    });
  }

The issue is that err is typed as grpcWeb.RpcError instead of grpcWeb.RpcError | null. The linter checks the if statement and detect it is not necessary, because the generated type states it is always a grpcWeb.RpcError , which is not true because it can be null if the request succeeds.

Can you adjust the generator to include the nullable type?

[sampajano]

@arluko Hi! Thanks for the report! This looks very reasonable. Lemme try to work on a fix. Thanks!

(although, this might be a breaking change to existing users so maybe i need to be a bit cautious about releasing :))

[sampajano]

@arluko Actually, on another thought, on the callback method, only one of err and response will be null, but NOT both at the same time.

In your code example, most people would probably want to do something like:

      this._apiClient.setFoo(request, {}, (err, response) => {
        if (err) {
          reject(err);
        } else {
          resolve(response); // <=============== resolve with response
        }
      });

But if i make both types nullable, then they have to check null for both of them, which makes it rather inconvenient..

like maybe:

      this._apiClient.setFoo(request, {}, (err, response) => {
        if (err) {
          reject(err);
        } else if (response) {
          resolve(response); // <=============== resolve with response
        } else { // <========== this will NEVER happen..
        }
      });

I'd think the latter inconvenience might outweight the warning that you see..

And given the fact that this will be a breaking change for existing users, i'm leaning towards not making the change..

@arluko What do you think? Is there an easier way for clients to work around the inconvenience? Thanks!

(@stanley-cheung CC FYI)

[arluko]

@sampajano As far as i know the only solution to setup typing which matches the runtime behavior is with discriminated unions (see example bellow), but this would required to change the interface of the callback which breaks every implementation.

// Example code snippet for a solution with discriminated unions: 
// see https://www.typescriptlang.org/docs/handbook/2/narrowing.html#discriminated-unions

type RpcResponse = { bar: string };
type RpcError = { msg: string };

type FooResult = { success: true, response: RpcResponse } | 
                 { success: false, error: RpcError };

class Demo {
    foo(result: FooResult): void {
        if (result.success) {
            console.log(result.response); // => response is `RpcResponse`
        } else {
            console.log(result.error); // => error is `RpcError`
        }
    }
}

Another way without completely breaking everything would be to type only one of the parameters as nullable, which is not (entirely) correct, but would allow to mitigate the issue you described in the second code snippet.

  setFoo(
    request: xyz_api_pb.FooRequest,
    metadata: grpcWeb.Metadata | null,
    callback?: (
      err: grpcWeb.RpcError | null,
      response: xyz_api_pb.FooResponse) => void // response is not typed as nullable to make usage simple
  ) {
    // ...
  }

What do you think about this option?

[arluko]

My current workaround is to prefix each of the error guards with an eslint-disable comment to turn off the rule for the next line. This works but you have to do this for (nearly) every service call and I had the hope that we can adjust the generated code somehow to mitigate this.

  this._apiClient.setFoo(request, {}, (err, response) => {
+   // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
    if (err) {
      reject(err);
    } else {
      resolve(response);
    }
  });

[sampajano]

@arluko Thanks so much for the ideas!

As far as i know the only solution to setup typing which matches the runtime behavior is with discriminated unions

I totally agree that the discriminated union is the better solution, but i don't think we can afford breaking every implementation..

Another way without completely breaking everything would be to type only one of the parameters as nullable, which is not (entirely) correct, but would allow to mitigate the issue you described in the second code snippet.

This is pretty smart! Although i do not really like the asymmetry (and incorrectness) either.. especially not everyone would check for error and response using the example i provided..

This works but you have to do this for (nearly) every service call and I had the hope that we can adjust the generated code somehow to mitigate this.

I agree 100% that this is very sub-optimal and definitely should be improved on. But since we're anyways going to make a breaking change, i think we should aim to do it right.. (else the gain does not warrant the cost (breaking the users) IMO.)

---

Regarding discriminated unions, I actually haven't worked much on Typescript myself.. Would something like the following work?

type FooResult = { error: null, response: RpcResponse } | 
                 { error: RpcError, response: null };

Thanks!!

[arluko]

@sampajano According to this example your suggestion should work and the types are resolved correctly.

[sampajano]

@arluko Thanks so much for providing the link to the TS Playground! Good to learn!

Although.. I've only just realized that this again requires making a breaking change to all clients, since it is now returning one object instead of two..

I'm not sure if there's a good fix without breaking all users (which we cannot afford)..

Ideas are welcome.. :)

[arluko]

@sampajano I think there is no good/correct solution without a breaking change. The only other way I can currently think of is the idea from https://github.com/grpc/grpc-web/issues/1309#issuecomment-1373228077, but I understand why you dont like it and I like it neither.

I have a workaround , so it is not a big issue for me anymore, but maybe you can consider it again if some other breaking changes are introduced into the project anyways.

[glerchundi]

What about doing a breaking change following more or less the same strategy as other major programming languages do? That is:

1. 1.5: Opt-in, could be under a flag while generating a code. Notify which one will be deprecated in the future.
2. 1.6: Opt-out, same approach. Notify if opted out.
3. 1.7: Release a new version informing about the deprecation and the removal of the flag that disbles the possibility to opt out.

WDYT?

[sampajano]

What about doing a breaking change following more or less the same strategy as other major programming languages do?

@glerchundi Thanks a lot for the suggestion! It makes a lot of sense to me!

Although note that this will involve creating and maintaining 2 runtime behaviors in the following code, which we happen to share with streaming RPC handling as well..

https://github.com/grpc/grpc-web/blob/e49389873d887d15ab2870288f620aa2f15b3b85/javascript/net/grpc/web/grpcwebclientbase.js#L246-L290

I think this will be a fairly involved change / process so unfortunately it won't be my priority at this moment.. If this issue gets a lot of demands i'll consider prioritizing it further.

In any case contributions are very welcome too! :)

---

When we do change it, my current preference is to change it to the following, as we've discussed above:

type RpcCallResponse = { error: null, response: RpcResponse } | 
                       { error: RpcError, response: null };

And we update rpcCall to:

    rpcCall<REQ, RESP> (
      method: string,
      request: REQ,
      metadata: Metadata,
      methodDescriptor: MethodDescriptor<REQ, RESP>,
      callback: (RpcCallResponse) => void
    ): ClientReadableStream<RESP>;

[sampajano]

Actually on another thought, would you be able to use thenableCall which does not suffer from the same typing issue?

[sampajano]

Closing for now due to inactivity. Feel free to reopen if you're still interested in following up :)