unary request and client stream promise support

[EduardoRFS]

trafficstars

Support then() when unary request or client stream, essentially both APIs are the ones that already exposes a callback, so I just removed the need for a callback and make the return of both thenable.

No breaking change, every test pass(and two new ones), the API only became more open, now allow to avoid a callback.

Usage example:

// with callback
it('with unary call', function(done) {
  registry = new CallRegistry(done, expected_calls, true);
  var message = { value: 'foo' };
  client.echo(message, options, function(err) {
    if (!err) { // do what if error happen? yeah nothing
      registry.addCall('response');
    }
  });
});

// async await version
it('with unary call', async function(done) {
  registry = new CallRegistry(done, expected_calls, true);
  var message = { value: 'foo' };
  await client.echo(message, options); // will throw if fail
  registry.addCall('response'); 
});

[thelinuxfoundation]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• User @EduardoRFS isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards, CLA GitHub bot

[EduardoRFS]

CLA?

[murgatroid99]

In general, we require API changes to go through our gRFC proposal process.

In addition, the problem I see here is that for existing users and anyone who uses the callback API, this will introduce new UnhandledPromiseRejectionWarnings whenever a call has an error. According to that warning's text, those will be fatal errors in some future Node version. When that happens, anyone who uses the callback API instead of the promise API will have their process exit every time a call has an error, even though they are "handling" it in the callback. That is unacceptable.

[EduardoRFS]

@murgatroid99

In general, we require API changes to go through our gRFC proposal process.

Even non breaking ones?

UnhandledPromiseRejectionWarnings whenever a call has an error

I already thought about it(when I ran the tests it was flooded with that), you can try removing that block and running tests.

if (typeof callProperties.callback === 'function') {
  // avoid UnhandledPromiseRejectionWarning
  promise.catch(() => {});
}

[murgatroid99]

Yes, we do require proposals for non-breaking API changes. That is most of the content in that proposal repository.

[EduardoRFS]

@murgatroid99 See it will take some time to write an proposal(probably more broadly using the "same" approach), so would be really be helpful if you could give me a feedback about making APIs PromiseLike

[murgatroid99]

One of the primary purposes of making a proposal is to get feedback about the design.

It does seem a little weird to me to not have the full Promise prototype here. Do you know how well PromiseLike objects interact with existing APIs such as Promise.all? Other than that, this solution looks like a good way to add promise support, but I am somewhat worried about increasing the complexity of these APIs. What are the benefits of adding this as compared with leaving it alone and leaving it up to users to "promisify" the API?

[EduardoRFS]

@murgatroid99 It should work with every API that supports promise, the base of a Promise it's a thenable(object that have a then). https://promisesaplus.com/

The advantage is that we can by default allow it to be a the standard library for basic usages without a lot of wrappers, wrappers are easy to do(not so much to maintain). But one problem is typescript support, with static generated code, that yes promisify works for it, but needs to run it on each APIs and is really easy to make the error code became cryptic, so is better with a native support. Also promises are the standard for asynchronous code nowadays. Using callbacks isn't even a option anymore for a lot of people.

I could fork it or make a wrapper, but then I need to fork the entire ecosystem(well I almost did).

• I'm waiting this to make a PR for grpc_tools_node_protoc_ts with the right types.
• One PR on @types/node for stream api that supports generics.
• If I could merge the one in @types/node I already have one more for this repo with the right types.

After everything I get a 100% typed code with an nice usage, without any major breaking change. ex: api.spec.ts.

Also it's easier for myself(and for everyone) if everything became part of the mainstream. Anyway, I will write the proposal today or tomorrow, it will be better written than that, then we can think better about it.

[gwer]

It's only been three years. Any news? 😸

Promises out of the box would be very helpful.