js-sdk icon indicating copy to clipboard operation
js-sdk copied to clipboard

Replacing callbacks to improve readability

Open SoTrx opened this issue 1 year ago • 10 comments

Describe the proposal

In it's current state, the code uses two main ways to handle asynchronous operations.

The HTTP implementation uses promises, and the GRPC implementation uses (or more likely "have to use") callbacks.

Using callbacks has always been a pain in JS, as we have to deal with both synchronous and asynchronous errors, leading to some "mandatory boilerplate". For example, in sidecar.ts

  static async isStarted(client: GRPCClient): Promise<boolean> {
    const callClient = await client.getClient(false);

    return new Promise((resolve, _reject) => {
      try {
        callClient.getMetadata(new Empty(), (err, _res: GetMetadataResponse) => {
          if (err) {
            return resolve(false);
          }

          return resolve(true);
        });
      } catch (_e) {
        return resolve(false);
      }
    });
  }
}

The purpose of this issue would be to replace most of the GRPC client callbacks with promises.

Although there is no performance benefit to doing this (nor any performance hit), it will greatly improve the readability of the code, especially as more complex features are added to the SDK (i.e Workflows), in which we may have to deal with nested callbacks.

To achieve this, we could use the promisify function (included in Node 8+).

On the same example, it would look something like this.


  static async isStarted(client: GRPCClient): Promise<boolean> {
    const callClient = await client.getClient(false);
    let isStarted = true;
    try {
      await promisify(callClient.getMetadata)(new Empty());
    } catch (e: unknown) {
      isStarted = false
    }
    return isStarted
}

I can see two ways in which this could be used. We could use the promisify function each time the GRPC client is used, which is simple.

Or we could wrap the whole GRPC client class in an adapter class, that "promisifies" all of the client's methods, which would be more DRY, but add a bit of extra maintenance.

What do you think ?

SoTrx avatar Apr 28 '23 13:04 SoTrx