-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Remove unnecessary catch-clauses for try-finally #967
Conversation
@@ -151,9 +151,6 @@ var Mixin = { | |||
try { | |||
this.initializeAll(); | |||
ret = method.call(scope, a, b, c, d, e, f); | |||
} catch (error) { | |||
// IE8 requires `catch` in order to use `finally`. | |||
errorToThrow = error; |
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 is quite tricky to see how the behavior still stays identical (not masking it in finally, see comment on later lines), so could this be simplified somehow.
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.
@plievone Oh wups, I was too fast there, that shouldn't be gone.
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.
Or wait no, that is correct, it simply throws it later.
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.
@syranide It is possible, perhaps preferred(?) to remove that catch, but the behavior stays the same because the last if (errorToThrow)
is not reached if thrown without catch so some variable renaming would help perhaps.
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 could even greatly simplify it I noticed.
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.
@spicyj Aha, then the comment is wrong.
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.
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.
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, the errors are significantly more confusing if the closeAll error bubbles up. I wasted a few hours debugging something because I'd removed the catches locally and did the wrong 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.
(See #975!)
Remove unnecessary catch-clauses for try-finally
Talked with @spicyj in the chat yesterday when he was trying to reproduce the idea that IE8 doesn't support
try {} finally {}
withoutcatch(e) {}
. He couldn't reproduce it and I can't either, no matter the craziness.After searching more I'm 99% convinced that this is actually false. The reason people incorrectly assume this is the case is because IE8 always reports uncaught exceptions at last
finally
. So if you confuse IE8 really well and get aThis command is not supported
, it will report that error and that error alone at the lastfinally
clause. Thus leading everyone to believefinally
is the cause and not the error itself.This seems to be corroborated here: knockout/knockout#545 (comment)
So unless you guys know for sure that this is an issue, I say we get rid of them (because some browsers actually mess up the exceptions when you re-throw them).
Credit goes to @spicyj for bringing it up.