-
Notifications
You must be signed in to change notification settings - Fork 27.4k
feat(errorHandlingConfig): make the length of URL refrence in error m… #15707
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,13 +51,19 @@ function minErr(module, ErrorConstructor) { | |
return match; | ||
}); | ||
|
||
message += '\nhttp://errors.angularjs.org/"NG_VERSION_FULL"/' + | ||
var url = 'http://errors.angularjs.org/"NG_VERSION_FULL"/' + | ||
(module ? module + '/' : '') + code; | ||
|
||
for (i = 0, paramPrefix = '?'; i < templateArgs.length; i++, paramPrefix = '&') { | ||
message += paramPrefix + 'p' + i + '=' + encodeURIComponent(templateArgs[i]); | ||
url += paramPrefix + 'p' + i + '=' + encodeURIComponent(templateArgs[i]); | ||
|
||
if (url.length > minErrConfig.urlMaxLength) { | ||
url = url.substr(0, minErrConfig.urlMaxLength - 3) + '...'; | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gkalpak Do you have any suggestion ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I suggest, you don't take it into account when calculating the URL length. And instead of appending |
||
} | ||
} | ||
|
||
message += '\n' + url; | ||
return new ErrorConstructor(message); | ||
}; | ||
} |
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.
Please add a note explaining the consequences this will have to longer URLs (e.g. will they break? will they lack information? will the stack traces be truncated?)
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.
Restricting this will definitely mess with the error parsing in the docs app. I was wondering if we should additionally to objectMaxDepth have arrayMaxLength? Because when you have an array with 1000 objects, then objectMaxDepth isn't going to help your shortening the error output
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.
Limiting the array length makes sense (although not directly related to this PR).
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.
The problem is that each parameter value is encoded using
encodeURIComponent
so when slice this encoded value it become invalid for some cases.
for example:
This link produced when i set urlMaxLength to 400
http://errors.angularjs.org/1.6.3-local+sha.48e0b69/$injector/modulerr?p0=n…cies%20as%20the%20second%20argument.%0Ahttp%3A%2F%2Ferrors.angularjs.org%2...
it look like it turn into
https://docs.angularjs.org/error/$injector/modulerr?p0=undefined
I will try to find a solution that slice the url without harming the encoded data.