This repository was archived by the owner on Apr 12, 2024. It is now read-only.
fix($log): don't parse error stacks manually outside of IE/Edge #15767
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gkalpak
reviewed
Mar 2, 2017
src/ng/log.js
Outdated
| // break apps. Other browsers display errors in a sensible way and some of them map stack | ||
| // traces along source maps if available so it makes sense to let browsers display it | ||
| // as they want. | ||
| if (arg.stack && (msie || /\bEdge\//.test($window.navigator.userAgent))) { |
There was a problem hiding this comment.
AFAICT, this is adding a new requirement that mocking $window on tests (directly or indirectly) relying on $log.error() must include navigator.userAgent. This might break some tests.
There was a problem hiding this comment.
This could be fixed by changing that into $window.navigator && $window.navigator.userAgent. WDYT?
There was a problem hiding this comment.
Works for me.
Also, I think it is a good idea to do this whole check once when instanciating the $log service:
var formatStackTrace = msie || ($window.navigator && /\bEdge\//.test($window.navigator.userAgent));
IE/Edge display errors in such a way that it requires the user to click in 4 places to see the stack trace. There is no way to feature-detect it so there's a chance of the user agent sniffing to go wrong but since it's only about logging, this shouldn't break apps. Other browsers display errors in a sensible way and some of them map stack traces along source maps if available so it makes sense to let browsers display it as they want. Fixes angular#15590
gkalpak
approved these changes
Mar 2, 2017
mgol
added a commit
that referenced
this pull request
Mar 3, 2017
IE/Edge display errors in such a way that it requires the user to click in 4 places to see the stack trace. There is no way to feature-detect it so there's a chance of the user agent sniffing to go wrong but since it's only about logging, this shouldn't break apps. Other browsers display errors in a sensible way and some of them map stack traces along source maps if available so it makes sense to let browsers display it as they want. Fixes #15590 Closes #15767
ellimist
pushed a commit
to ellimist/angular.js
that referenced
this pull request
Mar 15, 2017
IE/Edge display errors in such a way that it requires the user to click in 4 places to see the stack trace. There is no way to feature-detect it so there's a chance of the user agent sniffing to go wrong but since it's only about logging, this shouldn't break apps. Other browsers display errors in a sensible way and some of them map stack traces along source maps if available so it makes sense to let browsers display it as they want. Fixes angular#15590 Closes angular#15767
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IE/Edge display errors in such a way that it requires the user to click in
4 places to see the stack trace. There is no way to feature-detect it so
there's a chance of the user agent sniffing to go wrong but since it's only
about logging, this shouldn't break apps. Other browsers display errors in
a sensible way and some of them map stack traces along source maps if available
so it makes sense to let browsers display it as they want.
Fixes #15590
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
What is the current behavior? (You can also link to an open issue here)
Stack traces don't have source maps applied in any browser. The stack trace is formatted by Angular and printed as a string.
What is the new behavior (if this is a feature change)?
Stack traces have source maps applied in Chrome/Safari. The received error is printed as-is instead of printing a transformed stack trace.
Does this PR introduce a breaking change?
Maybe? The format of logged exceptions is not specified in the docs but there is a chance someone depends on it always being a string.
Please check if the PR fulfills these requirements
Docs have been added / updated (for bug fixes / features)Other information: