Simplify rejection handler to propagate error by re-throwing reason.

[sparhami]

No description provided.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[googlebot]

CLAs look good, thanks!

[dvoytenko]

Do we have a guarantee that reason would always be an Error?

[sparhami]

Interestingly enough, you can throw anything you want in JavaScript.

[erwinmombay]

throwing a string literal (strings etc, non Error) is generally bad practice though

[dvoytenko]

Yeah, we've been trying to avoid that.
On Oct 8, 2015 9:55 PM, "erwin mombay" notifications@github.com wrote:

throwing a string is generally bad practice though

—
Reply to this email directly or view it on GitHub
#554 (comment).

[sparhami]

By the same token you shouldn't reject a Promise with something that isn't an Error. If you converted the code to async/await, you wouldn't throw a string.

[erwinmombay]

hmm thats a good point. but i generally think of promise rejection as a way to cancel all the handlers subsequent to the current one (breaking the composition) and not an analog of actual error handling.

[dvoytenko]

Agree. Would gladly accept a PR that replaces reject(string) with reject(new Error()). But I do wonder:
(a) We often not in control of rejection reason, e.g. through some other API.
(b) The worst case, I think, is when someone does Promise.reject(), i.e. with reason=undefined.

Any thoughts on these?

[erwinmombay]

@sparhami friendly ping to follow up on @dvoytenko

[sparhami]

I'm not sure why it matters if you don't have control over the upstream. You can throw undefined just fine. This change does not introduce any change in behavior.

I would encourage you to change the upstream to throw an Error instead of a string. It is probably a good idea because then you can tell what actually caused the rejection chain rather than getting a string and having to search for it in the code base.

If you would like, I could create a separate PR for that change.

[dvoytenko]

Apologies, missed the last report. This sounds good. LGTM. And we'd definitely would welcome a PR to update rejections to use an exception.