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

[JS] Fixed instanceof for native functions #2441

Closed
wants to merge 3 commits into
base: development
from

Conversation

Projects
None yet
4 participants
@Blank101
Contributor

Blank101 commented Dec 8, 2013

As part of the ES5 spec (http://ecma262-5.com/ELS5_HTML.htm#Section_11.4.3), native functions return "object" when typeof is called on them. Chrome, Firefox, Opera implement the spec incorrectly and return "function" so there are no problems, but at least iOS Safari (possibly desktop Safari) implement the spec correctly and return "object". This leads to cast throwing errors if you are working with native DOM objects.

@ncannasse

This comment has been minimized.

Member

ncannasse commented Dec 8, 2013

Could you give references on Safari not respecting this behavior?

@Simn

This comment has been minimized.

Member

Simn commented Dec 8, 2013

Note that this breaks stuff:

TypeError: Expecting a function in instanceof check, but got 0
    at Function.js.Boot.__instanceof (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:4842:21)
    at Object.unit.TestReflect.$extend.is (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:9740:20)
    at Object.unit.TestReflect.$extend.testIs (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:9702:8)
    at Function.unit.Test.main (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:5617:12)
    at [eval]:1:49
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:456:26)
    at evalScript (node.js:532:25)
    at startup (node.js:80:7)
    at node.js:901:3
Test.hx:224: ABORTED : TypeError: Expecting a function in instanceof check, but got C(0,hello) in unit.TestSerialize.test
Test.hx:227: STACK :
TypeError: Expecting a function in instanceof check, but got C(0,hello)
    at Function.js.Boot.__instanceof (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:4842:21)
    at t.fileName (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:10097:17)
    at Object.unit.TestSerialize.$extend.doTestEnums (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:10099:4)
    at Object.unit.TestSerialize.$extend.test (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:10019:8)
    at Function.unit.Test.main (/home/travis/build/HaxeFoundation/haxe/tests/unit/unit.js:5617:12)
    at [eval]:1:49
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:456:26)
    at evalScript (node.js:532:25)
    at startup (node.js:80:7)
Test.hx:187: DONE [3509 tests]
@Blank101

This comment has been minimized.

Contributor

Blank101 commented Dec 8, 2013

Ok, a slightly more complicated example is needed but here is an example of the failure: http://jsfiddle.net/WQp7P/1/

package ;
class CastFailure {

    public static function main () {
        try {
            var elem:js.html.Element = js.Browser.document.createDivElement();
            var div:js.html.DivElement = cast(elem, js.html.DivElement);
            js.Browser.window.alert("No cast error");
        } catch (e:Dynamic) {
            js.Browser.window.alert("Err: " + e);
        }
    }

}

You should get no cast error for most (if not all) desktop browsers, but open that from an iPhone and you will get an error.

@mockey

This comment has been minimized.

Contributor

mockey commented Dec 8, 2013

Yep, I get the cast error with my old Firefox 11, just like here: #2297

@Blank101

This comment has been minimized.

Contributor

Blank101 commented Dec 9, 2013

Newest commit seems to pass unit tests. Although it's not the nicest solution it does get the job done. I tested this with iOS 7 Safari and BB10 - both of which failed before and are now succeeding.

@ghost ghost assigned ncannasse Jan 6, 2014

@Blank101

This comment has been minimized.

Contributor

Blank101 commented Feb 18, 2014

The current fix only works for Elements, but it should work for all native functions. It seems whitelisting the .toString() results is an uphill battle which is the only way I can find to detect native functions.

Using "instanceof" seems like a better approach (as I did in the first commit), but that seems to be too lenient. Will have to look into this some more...

@ncannasse

This comment has been minimized.

Member

ncannasse commented Feb 18, 2014

We cannot accept the commit as it, since it seems quite arbitrary to rely on the string representation.

@Blank101

This comment has been minimized.

Contributor

Blank101 commented Feb 18, 2014

Agreed.

@Blank101

This comment has been minimized.

Contributor

Blank101 commented Mar 3, 2014

I'm going to close this as I can't see a good way around this problem.

@Blank101 Blank101 closed this Mar 3, 2014

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