Skip to content

fix(zone.js): fix #27840, don't wrap uncaught promise error.#31443

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:uncaught-promise-error
Closed

fix(zone.js): fix #27840, don't wrap uncaught promise error.#31443
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:uncaught-promise-error

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion commented Jul 7, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #27840

zone.js will catch uncaught promise rejection, such as this case.

Promise.reject(new Error('test'));

And when zone onHandleError catch the error, the error will look like

{
  message: 'Uncaught (in promise): test',
  reason: Error('test')
}

because zone.js wrap the original error into a new Error with the reason property equals to the original error. It will make developer hard to use, because developer expect the original error will be caught in error handler.

What is the new behavior?

If the uncaught Promise rejection is instanceof Error or has a message property.
To keep the backward compatibility, I also set the reason property to the error.

Does this PR introduce a breaking change?

  • Yes
  • No

@jasonaden jasonaden added the area: zones Issues related to zone.js label Jul 9, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Jul 9, 2019
@JiaLiPassion JiaLiPassion force-pushed the uncaught-promise-error branch from b625ba9 to fb18cd2 Compare July 22, 2019 13:39
@JiaLiPassion JiaLiPassion requested a review from a team July 22, 2019 13:39
@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 22, 2019
@ngbot
Copy link
Copy Markdown

ngbot Bot commented Jul 22, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci-codefresh-bazel" is failing
    pending status "ci/circleci: test_docs_examples" is pending
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jul 22, 2019

http://test/OCL:259425227:BASE:259425241:1563836498178:e13d7cd9

@mhevery mhevery closed this in 2bb9a65 Jul 24, 2019
mhevery pushed a commit that referenced this pull request Jul 24, 2019
@JiaLiPassion @mhevery
fix(zone.js): don't wrap uncaught promise error. (#31443)
0440dc9 
Close #27840

PR Close #31443
alexeagle added a commit to alexeagle/angular that referenced this pull request Jul 30, 2019
@alexeagle
Revert "fix(zone.js): don't wrap uncaught promise error. (angular#31443…
374607a 
…)"

This reverts commit 2bb9a65.

It breaks tests in google3 which rely on the error handling behavior.
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
@alexeagle @AndrewKushnir
Revert "fix(zone.js): don't wrap uncaught promise error. (#31443)" (#…
975917b 
…31918)

This reverts commit 2bb9a65.

It breaks tests in google3 which rely on the error handling behavior.

PR Close #31918
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
@alexeagle @AndrewKushnir
Revert "fix(zone.js): don't wrap uncaught promise error. (#31443)" (#…
03e8a31 
…31918)

This reverts commit 2bb9a65.

It breaks tests in google3 which rely on the error handling behavior.

PR Close #31918
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@JiaLiPassion @sabeersulaiman
fix(zone.js): don't wrap uncaught promise error. (angular#31443)
1a8eabc 
Close angular#27840

PR Close angular#31443
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@alexeagle @sabeersulaiman
Revert "fix(zone.js): don't wrap uncaught promise error. (angular#31443…
4f7dd19 
…)" (angular#31918)

This reverts commit 2bb9a65.

It breaks tests in google3 which rely on the error handling behavior.

PR Close angular#31918
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Sep 15, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mhevery mhevery mhevery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

4 participants

@JiaLiPassion @mhevery @jasonaden @googlebot