Skip to content

fix(zone.js): do not silence uncaught promise rejections when the finally is called - #45449

Closed
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/43206-silence-finally
Closed

arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/43206-silence-finally

Conversation

@arturovt

Copy link
Copy Markdown
Contributor

The native promise implementation logs unhandled promise rejections after onFinally
has been called. We may rely on the scheduledByFinally argument, which is false by
default, to avoid creating a breaking change; this will also allow us to reduce the number of
changes. The finally function should not silence unhandled promise rejections, because
they're not silenced in any implementation. If the scheduleResolveOrReject is called within
the finally we won't clear unhandled rejections. We'll keep the same behaviour if the
scheduleResolveOrReject is called within the then.

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

Issue Number: #43206

Does this PR introduce a breaking change?

  • Yes
  • No

@pullapprove
pullapprove Bot requested a review from JiaLiPassion March 26, 2022 07:22
@dylhunn dylhunn added the area: zones Issues related to zone.js label Mar 28, 2022
@ngbot ngbot Bot added this to the Backlog milestone Mar 28, 2022
@arturovt

Copy link
Copy Markdown
Contributor Author

@JiaLiPassion friendly ping :)

@arturovt
arturovt force-pushed the fix/43206-silence-finally branch from 53bff0b to bfeb553 Compare June 18, 2022 19:08
@arturovt
arturovt force-pushed the fix/43206-silence-finally branch 2 times, most recently from e0baa6c to 411e235 Compare June 19, 2022 20:21
…nally` is called

The native promise implementation logs unhandled promise rejections after `onFinally`
has been called. We may rely on the `scheduledByFinally` argument, which is `false` by
default, to avoid creating a breaking change; this will also allow us to reduce the number of
changes. The `finally` function should not silence unhandled promise rejections, because
they're not silenced in any implementation. If the `scheduleResolveOrReject` is called within
the `finally` we won't clear unhandled rejections. We'll keep the same behaviour if the
`scheduleResolveOrReject` is called within the `then`.

PR Close angular#43206
@arturovt
arturovt force-pushed the fix/43206-silence-finally branch from 411e235 to 5bd2487 Compare June 19, 2022 20:35

@JiaLiPassion JiaLiPassion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for the fix.

@JiaLiPassion JiaLiPassion added target: minor This PR is targeted for the next minor release action: merge The PR is ready for merge by the caretaker labels Jun 19, 2022
@AndrewKushnir

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit cb57cdb.

@arturovt
arturovt deleted the fix/43206-silence-finally branch June 22, 2022 20:28
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Jun 22, 2022
@AndrewKushnir

Copy link
Copy Markdown
Contributor

FYI, this PR will be reverted via #46461, since it was causing some issues in Google's codebase (in tests). @arturovt could you please create a new PR with this change, so that we can try to land it again (after some prep work in Google's codebase)? Thank you.

AndrewKushnir added a commit that referenced this pull request Jun 22, 2022
… the `finally` is called (#45449)" (#46461)

This reverts commit cb57cdb.

PR Close #46461
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants