-
Notifications
You must be signed in to change notification settings - Fork 3
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
Account for recursive error cycles. #39
base: master
Are you sure you want to change the base?
Account for recursive error cycles. #39
Conversation
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.
Is there a reason we expect the same error to ever be triggered?
If not, maybe a normal WeakMap would be easier here...
@@ -7,6 +7,11 @@ let squelchedLabels; | |||
export let squelchErrorHandlerFor = null; | |||
export let unsquelchAllErrorHandlers = null; | |||
|
|||
const excludeValue = (array, val) => { |
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.
Hmm, where is this used?
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.
It was, uhh… that kind of morning. I'll remove it.
const desc = reason.toString(); | ||
const pos = recentReasons.indexOf(desc); | ||
if (pos !== -1) { | ||
recentReasons = recentReasons |
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.
This is basically making an LRU type of thing?
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.
Yeah, basically aiming to keep the same semantics we had before, but for multiple errors instead of just one. So wherever it appears in the list, remove it. Happy to change the data structure; this was the naïve/first-pass thing.
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.
Yeah, a WeakMap would presumably be fine. I don't know what the value would be – true
, maybe? – but it would work.
I can pair with you and show you a couple places I've seen errors recur in our app, but the most annoying one others are likely to have seen it is a cascade of "Glimmer transaction was already begun" that can show up in some weird and surprising spots.
@@ -7,6 +7,11 @@ let squelchedLabels; | |||
export let squelchErrorHandlerFor = null; | |||
export let unsquelchAllErrorHandlers = null; | |||
|
|||
const excludeValue = (array, val) => { |
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.
It was, uhh… that kind of morning. I'll remove it.
const desc = reason.toString(); | ||
const pos = recentReasons.indexOf(desc); | ||
if (pos !== -1) { | ||
recentReasons = recentReasons |
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.
Yeah, basically aiming to keep the same semantics we had before, but for multiple errors instead of just one. So wherever it appears in the list, remove it. Happy to change the data structure; this was the naïve/first-pass thing.
Fixes #38.
@rwjblue, I'm not exactly sure what to write in the test for this. Happy to add it once I know what I should put there!