-
Notifications
You must be signed in to change notification settings - Fork 0
Unhandled promise rejections for the 1.5.10 release #1
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
base: v1.5.10_fix_unhandled_promises
Are you sure you want to change the base?
Unhandled promise rejections for the 1.5.10 release #1
Conversation
@@ -242,10 +279,14 @@ function $$QProvider() { | |||
* @param {function(function)} nextTick Function for executing functions in the next turn. | |||
* @param {function(...*)} exceptionHandler Function into which unexpected exceptions are passed for | |||
* debugging purposes. | |||
* @param {=boolean} errorOnUnhandledRejections Whether an error should be generated on unhandled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor typo @param {boolean}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall these changes look fine what maybe more important is for us to watch the automation integration tests and to look for possible issues after the new angular.js is compiled and deployed into SE to follow up on potential issues.
@@ -2112,6 +2125,51 @@ describe('q', function() { | |||
}); | |||
|
|||
|
|||
|
|||
describe('when exceptionHandler is called', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is more relevant to be included in SE source code as we don't normally run angular ng library test source code we just assume the angular library "just works".
Potentially we don't need that many just a simple check that our built in exception handlers will get called by default should be sufficient. If this is coming from the angular PR you're back porting then I'd leave this in.
I would add Anthony to this PR so he's aware of the changes that will be brought in please include the PR. We will also be separately reviewing the new PR for the angular.js script in SE once it appears in that repo as well. |
I'm waiting for him to accepts an invitation. I couldn't add the repo into our organization because it requires extra permissions. So I had to resort to forking it into my work account directly. Maybe it could be transfered there later |
This PR contains changes on top of the 1.5.10 release that enable catching unhandled promise rejections in the $exceptionHandler decorator. Issue: angular#13653
The lowest version of official releases with the changes is 1.6.0. Migration from 1.5.10 (which simple engagements is using) to 1.6.0 will require a lot of code changes, so this is an alternative approach where we take the changes and apply them directly to 1.5.10 in order to get the new feature without migration.
The PR and commit below refer to the same code that was originally developed by the google team to address this problem (commit is a squash commit from 1.6.0 while the PR is an original PR for the issue that was closed and removed)
https://github.com/angular/angular.js/pull/13662/files
angular@c9dffde