Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unary request and client stream promise support #981

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

EduardoRFS
Copy link

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
Copy link

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.

Regards,
CLA GitHub bot

@EduardoRFS
Copy link
Author

CLA?

@murgatroid99
Copy link
Member

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
Copy link
Author

EduardoRFS commented Aug 5, 2019

@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
Copy link
Member

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

@EduardoRFS
Copy link
Author

EduardoRFS commented Aug 5, 2019

@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
Copy link
Member

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
Copy link
Author

EduardoRFS commented Aug 6, 2019

@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
Copy link

gwer commented Nov 20, 2022

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

Promises out of the box would be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants