Movatterモバイル変換

$q: Unhandled rejections should not be stringified#14631

Closed

gkalpak added component: $q frequency: high severity: inconvenient type: feature labels

gkalpak added this to the1.6.1 milestone

Copy link

Member

gkalpak commentedDec 20, 2016

BTW, it would made our life a tiny bit easier if you followed ourcommit message guidelines 😉

Copy link

ContributorAuthor

graingert commentedDec 20, 2016

@gkalpak will do once I've got the tests working. Also your URL is out of date: you wanthttps://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format

gkalpak reviewed

src/ng/q.js

		@@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
		if (!toCheck.pur) {
		toCheck.pur = true;
		var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);

Copy link

Member

gkalpakDec 20, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Here is another idea (either fortoDebugString() or just here):
IftoCheck.value has atoString() method that is not the defaultObject.prototype.toString(), then use that for serializing it. Something similar tostringify.

graingert force-pushed thepatch-3 branch 2 times, most recently from6e2531b to013779cCompare

December 20, 2016 12:58

Copy link

Member

gkalpak commentedDec 20, 2016

Thx! (My URL was fine btw - just intentionally pointing to the supersection of yours 😛)

Copy link

ContributorAuthor

graingert commentedDec 20, 2016•
edited
Loading

@gkalpak ah it didn't go to a section for me at all. (it works now)

graingert force-pushed thepatch-3 branch 5 times, most recently fromc014f10 to1654b47Compare

December 20, 2016 15:28

Copy link

ContributorAuthor

graingert commentedDec 20, 2016

@gkalpak and we're green!

Copy link

Member

gkalpak commentedDec 20, 2016

@graingert, awesome! Now on to that commit message 😛

gkalpak suggested changes

Copy link

Member

gkalpak left a comment

Choose a reason for hiding this comment

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

I left some comments, but the general idea LGTM.

		var exceptionStr = toDebugString(exceptionEg);
		var fixtures = [
		{
		type: 'exception',

Copy link

Member

gkalpakDec 20, 2016

Choose a reason for hiding this comment

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

Change "exception" to "Error object".

Copy link

ContributorAuthor

graingertDec 20, 2016

Choose a reason for hiding this comment

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

Cool

		var type = fixture.type;
		var value = fixture.value;
		var expected = fixture.expected;
		it('should log an unhandled' + type + ' rejected promise', function() {

Copy link

Member

gkalpakDec 20, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Instead of repeatingtype in allit blocks, create adescribe block for each fixture and puttype in its description.

Copy link

ContributorAuthor

graingertDec 20, 2016

Choose a reason for hiding this comment

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

Ah makes sense

		forEach(fixtures, function(fixture) {
		var type = fixture.type;
		var value = fixture.value;
		var expected = fixture.expected;

Copy link

Member

gkalpakDec 20, 2016

Choose a reason for hiding this comment

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

Extra points for putting some space beforeit blocks 😃

Copy link

Member

gkalpakDec 21, 2016

Choose a reason for hiding this comment

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

Ordescribe blocks...

		expected: {
		reason: 'Possibly unhandled rejection: foo'
		}
		}

Copy link

Member

gkalpakDec 20, 2016

Choose a reason for hiding this comment

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

A couple more fixtures would be in order imo. E.g.:

One with a non-Error object.
One with an object that "extends"Error.

Copy link

ContributorAuthor

graingertDec 20, 2016

Choose a reason for hiding this comment

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

Yeah makes sense

gkalpak added needs: test needs: work and removed needs: test labels

graingert force-pushed thepatch-3 branch 3 times, most recently fromc75796b to1dd6336Compare

December 21, 2016 11:05

Copy link

ContributorAuthor

graingert commentedDec 21, 2016

@gkalpak done

gkalpak approved these changes

Copy link

Member

gkalpak left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks, but otherwise LGTM.

		}
		},
		{
		type: 'plain value',

Copy link

Member

gkalpakDec 21, 2016

Choose a reason for hiding this comment

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

Now everyone is capitalized except forplain value. Just because it is nothing more than a plain value, doesn't mean you have to treat it like a 2nd-class citizen 😛

Copy link

ContributorAuthor

graingertDec 21, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I went with all lower case, and more accurate names

		forEach(fixtures, function(fixture) {
		var type = fixture.type;
		var value = fixture.value;
		var expected = fixture.expected;

Copy link

Member

gkalpakDec 21, 2016

Choose a reason for hiding this comment

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

Ordescribe blocks...

		var type = fixture.type;
		var value = fixture.value;
		var expected = fixture.expected;
		describe(type, function() {

Copy link

Member

gkalpakDec 21, 2016

Choose a reason for hiding this comment

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

Nit: Using something like'with ' + type will make the description read better (in case of an error for example).

graingert force-pushed thepatch-3 branch from1dd6336 to7d681d3Compare

December 21, 2016 12:39

Copy link

ContributorAuthor

graingert commentedDec 21, 2016

@gkalpak done

gkalpak approved these changes

Copy link

Member

gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait for some feedback from others as well.
_{(In the mean time, feel free to fix up that commit message 😁)}

Copy link

ContributorAuthor

graingert commentedDec 21, 2016

@gkalpak I did fix the commit message... What's wrong with it?

Copy link

Member

gkalpak commentedDec 21, 2016•
edited
Loading

Actual:

fix($q): Add traceback to unhandled promise rejections, Fixes:#14631

Expected something like:

fix($q): add traceback to unhandled promise rejections
Fixes#14631

(The change may seem trivial, but the format is important, because we auto-generate the changelog from these commit messages. So small deviations can make a lot of difference in the resulting changelog.)

Copy link

Member

mgol commentedDec 21, 2016

@graingert Thanks for the PR, this is awesome. 👏

I don't have any remarks apart from the incorrect commit message format.

fix($q): Add traceback to unhandled promise rejections

d73fdde

Fixes:angular#14631

Copy link

ContributorAuthor

graingert commentedDec 21, 2016

@gkalpak commit message fixed

graingert force-pushed thepatch-3 branch from7d681d3 tod73fddeCompare

December 21, 2016 14:15

mgol suggested changes

Fromhttps://docs.angularjs.org/api/ng/service/$exceptionHandler

src/ng/q.js

		@@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
		if (!toCheck.pur) {
		toCheck.pur = true;
		var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
		exceptionHandler(errorMessage);
		if (toCheck.value instanceof Error) {
		exceptionHandler(toCheck.value, errorMessage);

Copy link

Member

mgolDec 21, 2016

Choose a reason for hiding this comment

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

Is this the correct order? It looks weird when the error message isafter the stack trace.

Copy link

ContributorAuthor

graingertDec 21, 2016•
edited
Loading

Choose a reason for hiding this comment

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

$exceptionHandler(exception,[cause]);

Param	Type	Details
exception	Error	Exception associated with the error.
cause(optional)	string	Optional information...

Copy link

Member

mgolDec 21, 2016

Choose a reason for hiding this comment

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

Hmm, OK. I mostly wondered because this:

looks a little weird.

Copy link

Member

mgolDec 21, 2016

Choose a reason for hiding this comment

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

There is nothing in the$exceptionHandler code that would make the order important; that's all of it:

function$ExceptionHandlerProvider(){this.$get=['$log',function($log){returnfunction(exception,cause){$log.error.apply($log,arguments);};}];}

I wonder what's the reason for the current documentation.@gkalpak,@petebacondarwin does any one of you know?

Copy link

ContributorAuthor

graingertDec 21, 2016•
edited
Loading

Choose a reason for hiding this comment

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

what about just doing

exceptionHandler(toCheck.value,'Possibly unhandled rejection')

with no ifs or errorMessage generation.

Copy link

Member

mgolDec 21, 2016

Choose a reason for hiding this comment

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

That sounds sensible but I'd still swap the order of operations. Note that currently we are trying to show the'Possibly unhandled rejection' message first and the error afterwards; we just put it all in one string argument so the stack trace gets lost. So I'd like:

exceptionHandler('Possibly unhandled rejection',toCheck.value);

but that would require updating the docs to no longer require the cause to be the second parameter (no code changes required as it already works as I mentioned before).

I'd just want to know if there's any reason for the current signature so let's see what others say. :)

Copy link

ContributorAuthor

graingertDec 21, 2016

Choose a reason for hiding this comment

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

@mgol my custom $exceptionHandlers are configured to pass the first argument as the Error value, as per the documentation.

I presume others follow the same documentation also.

Copy link

Member

mgolDec 21, 2016•
edited
Loading

Choose a reason for hiding this comment

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

@graingert Is the issue that some automated tools might only pick up the first argument and discard others, losing the error context? Because otherwise, in Angular source, the order influences only one thing - the order in which messages/errors are logged. And it makes more sense to log the summary first and the error second. In other words, the following:

exceptionHandler('Possibly unhandled rejection',toCheck.value);

is analogous to:

exceptionHandler('Possibly unhandled rejection: '+toDebugString(toCheck.value));

just with more details printed to the console as the error stack is not lost.

Copy link

ContributorAuthor

graingertDec 21, 2016

Choose a reason for hiding this comment

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

@mgol swapping the types of the parameters of $exceptionHandler is another issue for another PR, and would be a breaking change.

Copy link

Member

mgolDec 21, 2016

Choose a reason for hiding this comment

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

OK, fair enough. In that case, maybe it's better to leave it as it is for now; your proposal to just write:

exceptionHandler(toCheck.value,'Possibly unhandled rejection');

sounds nice but we pass other data types throughtoDebugString and we'd lose it in that case. If we want to tweak that, it would be better to tackle that separately after this PR lands.

mgol approved these changes

dcherman reviewed

src/ng/q.js

		@@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
		if (!toCheck.pur) {
		toCheck.pur = true;
		var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
		exceptionHandler(errorMessage);
		if (toCheck.value instanceof Error) {

Copy link

Contributor

dchermanDec 21, 2016

Choose a reason for hiding this comment

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

Though I don't think it necessarily a concern here, do we care that this will not identify errors that originate from an iframe? Don't know how common of a use case that is.

Copy link

Member

mgolDec 21, 2016

Choose a reason for hiding this comment

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

I doubt it's common but if it turns out to be an issue we can revisit.

Ideally we would convert to a string (viatoDebugString) only certain types of values and log others unchanged. The problem is that then we'd have to passPossibly unhandled rejection andtoCheck.value as separate arguments to$exceptionHandler and if we do it in a documented way (error first, (optional) reason second) the message would get swapped from e.g.:

'Possibly unhandled rejection: MY_THING'

to:

'MY_THING' 'Possibly unhandled rejection'

which is less pleasant.

We can discuss it but let's do it in a separate issue if desired.

mgol closed this in316f60f

mgol pushed a commit that referenced this pull request