try/finally statements need an intermediate catch expression in IE8 #545

Closed
wants to merge 7 commits into
from
@christianp

IE8 complains with a "This command is not supported" error when you have a try{}finally{} expression without a catch(e){} bit before the finally{}.
This adds a catch(pseudoError){} expression to all the try/finally expressions I could find.
I haven't remade the minified ouput because I don't have closure compiler.

christianp and others added some commits Jun 27, 2012
@christianp christianp add empty catch{} expression to try/finally to placate IE8
IE8 complains with a "This command is not supported" error when you have a try{}finally{} expression without a catch(e){} bit before the finally{}.
This adds a catch(pseudoError){} expression to all the try/finally expressions I could find.
I haven't remade the minified ouput because I don't have closure compiler.
baa7b98
@SteveSanderson SteveSanderson Allow template names to be selected as a function of the entire bindi…
…ng context
ff06d8b
@christianp christianp add empty catch{} expression to try/finally to placate IE8
IE8 complains with a "This command is not supported" error when you have a try{}finally{} expression without a catch(e){} bit before the finally{}.
This adds a catch(pseudoError){} expression to all the try/finally expressions I could find.
I haven't remade the minified ouput because I don't have closure compiler.
b074c42
@christianp christianp Merge branch 'master' of github.com:christianp/knockout 0eabe11
@christianp christianp Merge branch 'master' of git://github.com/SteveSanderson/knockout
Conflicts:
	src/binding/defaultBindings.js
18c8623
@christianp christianp add empty catch() statements so IE doesn't complain bad6530
@wachunga

+1 for this fix. In IE8, I get "This command in not supported" in 2.1 on line 1206. Adding an empty catch fixes the issue.

@wachunga

After more investigation, I retract my +1. The empty catches actually squelch useful error messages.

@christianp

I suppose catch(e){throw e} would also work, and allow error messages to bubble up.

@Munter

+1 for this patch from here as well. My application actually won't launch at all in IE8 because of this. Now switching to a patched version

@davidkhess

I ran into this bug at one point and vaguely remember discovering this was a spurious error caused by errors elsewhere in my Javascript that was then minified together with Knockout. It appears the IE8 parser can get confused in odd ways.

Again, sorry for the vague recollections but I think I figured this out because I tried to recreate this error with an isolated example and could not.

I don't know if that helps anyone here but it's worth investigating.

@SpikeTheMaster

This patch does not seem to have fixed it for me, the same error is still thrown by IE. Am struggling to find out to work out what I can do to fix my page. Might be another script error elsewhere, but I am not minifying the code together as @davidkhess is.

@SpikeTheMaster

Actually, I did some more investigation and it looks like the catch must be empty (or at least not rethrow the error), otherwise it will just throw the "This command is not supported" error once again.

Perhaps the catch could check the type of the error message before re throwing it?

@SteveSanderson

In order for us to be able to evaluate this, would you be able to create a small repro of a scenario where the problem occurs, using jsFiddle.net?

Without being able to see the error occurring, we unfortunately don't have any way of verifying the proposed solution, or of writing an automated test to ensure it keeps on working.

Thanks!

@SpikeTheMaster

I have made an attempt at http://jsfiddle.net/Spike/JNzsd/10/

IE v8.0.6001

From what I have fiddled with it looks like if you delete the sections:

<!-- ko if: $root.isStillDiscretionary -->
<!-- ko template:{foreach: discretionaryManagerQuestions, beforeRemove: hideWebsite, afterAdd: showWebsite} -->

The error does not get thrown..

So the fix for me was to put empty catches before the finally blocks(from the latest code in github)

Thanks for looking into this, knockout is really a great library!

@rniemeyer
Knockout.js member

Aside from the try/catch issue, you are likely running into an issue with IE stripping/moving comments in certain scenarios. I don't have access to IE at this very moment, but if you look at the rendered markup in the dev tools you will see that the comments are not likely where you expected and they are no longer properly nested as KO would expect for containerless bindings.

One option is to use tbody tags around your rows. It is legal for a table to contain multiple tbody tags.

@rlarno

I got the same error here, but is was due to IE 8 not supporting Array.filter. I had a click event bound to a method on my observable and in that method I used the Array.filter method.

Adding compatibility code for the method fixed the issue.

note: the error was in my code, IE 8 does (seem to) support the try {} finally{} syntax just fine. IE8 does not seem to properly be able to show the real error or the originating line of the error.

@droarty

I had the same error and expected the patch to be in the latest version of Knockout but was surprised that it was not... I tried christianp's fix and it worked perfectly.

Any chance this could be added to the next release?

@davidkhess

I think @rlarno has it correct. What is really happening is an exception is happening in code outside of Knockout but IE8 is (due to a bug) reporting the error in this clause with "This command is not supported".

The real fix is to try and track down where the error outside of Knockout is happening. It might help to try and recreate the same steps in a different browser such as Chrome and see if you spot the real exception there.

@mj1856

had the same problem, and @rlarno's solution fixed it for me.

@rudyryk

I'm facing this issue too and have to patch Knockout at the moment.

@rlarno
@rudyryk

As I've found out the problem was not about try..finally, but it's still about some edge case with IE7-8, because in any other browser script works fine. IE8 raises exception in evaluateImmediate at line var newValue = readFunction.call(evaluatorFunctionTarget); while trying to evaluate computed(). So that's another issue.

@garromark

I spent quite awhile working away at this problem for myself. The try...finally codeblock is indeed throwing an exception without being caught in IE8. The edge case I encountered was that the "attr" data-bind cannot change the "type" property of a DOM element in IE8.

Still, I think that exception handling should be considered a bug for IE8 compatibility in my humble opinion.

Relevent text about IE8:
The type property is read/write-once, but only when an input element is created with the createElement method and before it is added to the document.

Relevent SO post:
http://stackoverflow.com/questions/18240063/ie8-throws-this-command-is-not-supported-error-when-using-knockout-for-data-bi

@mbest
Knockout.js member

The problem appears to be that IE8 doesn't report the correct source of the error when a try/finally is being used. This causes the confused belief that the try/finally is causing the error. According to http://webbugtrack.blogspot.com/2007/11/bug-184-catch-to-try-catch-finally-in.html, IE 6-7 also has a problem with try/finally.

@mbest mbest closed this Nov 2, 2013
@syranide syranide referenced this pull request in facebook/react Jan 24, 2014
Merged

Remove unnecessary catch-clauses for try-finally #967

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment