BladeRunnerJS / topiarist

Topiarist provides tree and shape-based type verification for JavaScript.
MIT License
22 stars 4 forks source link

Error implementing interface in IE8 in a complex inheritance hierarchy #11

Closed fauna5 closed 9 years ago

fauna5 commented 10 years ago

The code example works fine in all other IEs and chrome, firefox. However in IE8 it throws an error. Whilst it is a wierd inheritance hierarchy, the inconsistency in IE8 is something a developer can spend ages trying to work out.

    test = {};

    test.RandomInterface = function(){};
    test.RandomInterface.prototype.random = function(){};

    test.CommonImplement = function(){};
    test.CommonImplement.prototype.commonInterfaceMethod = function(){};

    test.Class = function(){};
    topiarist.extend(test.Class, test.RandomInterface);
    topiarist.inherit(test.Class, test.CommonImplement);
    test.Class.prototype.classMethod = function(){};
    test.Class.prototype.commonInterfaceMethod = function(){};
    test.Class.prototype.random = function(){};

    test.ClassImplementor = function(){};
    topiarist.extend(test.ClassImplementor, test.Class);
    topiarist.inherit(test.ClassImplementor, test.CommonImplement);
    test.ClassImplementor.prototype.myMethod = function(){};
    test.ClassImplementor.prototype.commonInterfaceMethod = function(){};
    test.ClassImplementor.prototype.classMethod = function(){};

    var div = document.createElement("div");
    div.innerHTML="loaded";
    document.body.appendChild(div);

    new test.ClassImplementor();

Switching these two lines: topiarist.extend(test.ClassImplementor, test.Class); topiarist.inherit(test.ClassImplementor, test.CommonImplement);

to topiarist.extend(test.ClassImplementor, test.CommonImplement); topiarist.inherit(test.ClassImplementor, test.Class);

fixes the issue. Again painful to work out on your own.

janhancic commented 10 years ago

Do we know when this is getting fixed? We just ran into the issue again.

andy-berry-dev commented 10 years ago

@janhancic no timings ATM. I've added https://github.com/BladeRunnerJS/brjs/issues/935 (which covers several issues in Topiarist) to our 1.0 release milestone.

ioanalianabalas commented 9 years ago

Hi @fauna5, I was unable to replicate this issue using exactly the code you provided. Can you see what I might have done wrong?

ioanalianabalas commented 9 years ago

@janhancic, I don't suppose you know how to replicate this issue as the code provided by @fauna5 doesn't seem to work for me.

janhancic commented 9 years ago

That's odd. Are you testing on IE8? I don't have any code handy that would trigger that bug right now. And unfortunately don't have the time to digg it up.

ioanalianabalas commented 9 years ago

Yes, I tested it in IE8. Well, let us know if you encounter it again. :)

andy-berry-dev commented 9 years ago

This could be related to http://yourls.caplin.com/c where there is a stack overflow in IE8 for modules with dependencies chains deeper than 12.

A comment from the internal related issue:

In bootstrap.js, the mergePackageBlock and requireAll functions are appended to the window object. In IE8, functions on the window object are limited to 12 calls in a single call stack. This means any modules with a dependency chain of 13 or greater depth will cause a maximum call stack error, as requireAll will be called 13 or more times.

ioanalianabalas commented 9 years ago

I have spoken to @fauna5 and seeing as how we could not reproduce the failure, this will be closed as a non-issue.