-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Reject back-to-back duplicate errors/messages #861
Conversation
Refs #435 |
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.
LGTM aside from one comment
src/raven.js
Outdated
@@ -1821,6 +1859,56 @@ function htmlElementAsString(elem) { | |||
return out.join(''); | |||
} | |||
|
|||
|
|||
function isOnlyOneTruthy(a, b) { | |||
return a && !b || b && !a; |
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'd prefer this alternative function body:
return !!(!!a ^ !!b);
The xor operator makes it more obvious what's going on IMO, and this will always return a boolean; current version has some edge cases where it won't like isOnlyOneTruthy('', '')
(returns empty string). Or if we're cool with always returning 0 or 1 instead of a boolean, we can further simplify to !!a ^ !!b
.
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.
Good suggestion, doing it.
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.
what about !!a != !!b
?
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.
what about !!a != !!b
Are they equivalent?
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 green is the best color for bikesheds
Integration tests incoming. |
if (isOnlyOneTruthy(ex1, ex2)) | ||
return false; | ||
|
||
ex1 = ex1.values[0]; |
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.
note to self/being paranoid - do we want to nullcheck values here first?
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.
We assemble these objects ourselves, and other pieces of the code make the same assumption so I feel it's safe:
e.g. here's _trimPacket
:
_trimPacket: function(data) {
// For now, we only want to truncate the two different messages
// but this could/should be expanded to just trim everything
var max = this._globalOptions.maxMessageLength;
if (data.message) {
data.message = truncate(data.message, max);
}
if (data.exception) {
var exception = data.exception.values[0];
exception.value = truncate(exception.value, max);
}
return data;
},
cc @getsentry/javascript