Skip to content
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

Merged
merged 1 commit into from
Feb 4, 2014
Merged

Remove unnecessary catch-clauses for try-finally #967

merged 1 commit into from
Feb 4, 2014

Conversation

syranide
Copy link
Contributor

Talked with @spicyj in the chat yesterday when he was trying to reproduce the idea that IE8 doesn't support try {} finally {} without catch(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 a This command is not supported, it will report that error and that error alone at the last finally clause. Thus leading everyone to believe finally 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.

@@ -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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spicyj @syranide If it is important to avoid rethrowing, one can remove this catch and it will work ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plievone Technically it can be removed, but it would change the behavior, and it seems like @spicyj had a good reason to think it's important to keep it this way.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See #975!)

@petehunt
Copy link
Contributor

@yungsters @tomocchino ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants