Replacing callbacks to improve readability

[SoTrx]

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 ?

[nav-D]

I want to do this if it gets approved. I am new to contributing to open sources so I'll need some support.

[XavierGeerinck]

Hi @nav-D / @SoTrx we discussed this in the team, and we would be opening - although being an aesthetic feature - to change this if a PR could be made that does take into account the test coverage as well and does not introduce breaking changes. Is that something either of you could take up?

[SoTrx]

Theoretically speaking, there should be no breaking changes. This is just removing complexity before it gets out of hand, as I have seen in past projects. Of course, this is just a suggestion, as it has absolutely no impact on the SDK features. (I'll help with this one day, I promise x) )

I'll check the code to be sure, and suggest a PR.

@nav-D, once I get the PR ready, would you help me getting it right ?

[SoTrx]

/assign

[nav-D]

@SoTrx Sure I’d love to help out.

[kendallroden]

@XavierGeerinck - is this still a good first issue for contributors? Looks like there was never a PR opened.

[XavierGeerinck]

Adding @shubham1172 I would say it could be, but it's a bit stale so we would prefer other issues first

[XavierGeerinck]

Thanks a lot for this PR! We had a discussion around it and are going to put it on hold. The reason for this is since the Node.JS ecosystem is becoming more fragmented daily with new runtimes such as Deno, Bun, .... with Bun having a good change of taking it over.

Introducing this PR would require us to scope the input of supporting additional runtimes as some are Node supported only while others don't carry this requirement.

[SoTrx]

That's a very good point !

[joebowbeer]

Node's promisify is a very simple wrapper and could easily be inlined, something like:

const promisify = fn => (...args) => new Promise((resolve, reject) => {
    fn(...args, (error, value) => {
      if (error) {
        reject(error);
      } else {
        resolve(value);
      }
  });
});

The reason for this is since the Node.JS ecosystem is becoming more fragmented daily with new runtimes such as Deno, Bun, .... with Bun having a good change of taking it over.

Yikes.

I think you're wildly over-estimating the fragmentation of Node. (Have we learned nothing from yarn?)

If Bun intends to be a drop-in replacement for NodeJS then by all means let it drop-in, but don't let it block useful enhancements like this.