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

fix(core): do not rethrow an error from ErrorHandler #13534

Merged
merged 1 commit into from
Mar 10, 2017

Conversation

DzmitryShylovich
Copy link
Contributor

@DzmitryShylovich DzmitryShylovich commented Dec 16, 2016

Closes #13159 and #8993

Issue description:
The origin of the issue is this line. After rethrowing we are getting unsubscribed and do not receive further errors. According to the comment this is required to fail on bootstrap error but I think this is outdated because there're several tests to ensure that bootstrap will fail even if ErrorHandler is not rethrowing the error.

If we still need to rethrow on bootstrap then another solution would be to set rethrowError to false after bootstrap.

@vicb vicb added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 29, 2016
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Feb 10, 2017
@chuckjaz chuckjaz removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 7, 2017
@chuckjaz
Copy link
Contributor

This change was reverted.

The refactor done in the last commit broke users who used implements ErrorHandler. The approved change before the last commit does not break implements.

@DzmitryShylovich
Copy link
Contributor Author

@chuckjaz you mean sub classes won't have access to rethrowError prop?

@chuckjaz
Copy link
Contributor

chuckjaz commented Mar 10, 2017

No. The issue is that we specifically marked it rethrowError as @internal which excludes it from the .d.ts. By removing that and making it private you made implements ErrorHandler an error because private rethrowError shows up in the .d.ts file and makes it so the class cannot be used as an interface.

@DzmitryShylovich
Copy link
Contributor Author

@chuckjaz hmm sorry. didn't think about that. here's non breaking commit #15077

@chuckjaz
Copy link
Contributor

Thanks! In the future, avoid refactoring after approvals because we run internal tests before we commit. The internal tests succeeded on your previous change but on the refactored change.

@vicb
Copy link
Contributor

vicb commented Mar 10, 2017

avoid refactoring after approvals

probably we should make further changes invalidate approvals

@DzmitryShylovich
Copy link
Contributor Author

I changed the commit message (according to Igor's guidelines) and decided to simplify the code a bit 😄

SamVerschueren pushed a commit to SamVerschueren/angular that referenced this pull request Mar 18, 2017
SamVerschueren pushed a commit to SamVerschueren/angular that referenced this pull request Mar 18, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@angular-automatic-lock-bot
Copy link

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 11, 2019
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: core Issues related to the framework runtime cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default ErrorHandler stops working after first error is handled
5 participants