[WIP] Reimplement removed constructor naming #43
Conversation
lib/util/util.inherit.js
Outdated
There was a problem hiding this comment.
why did you change this? namedFn should always == true
There was a problem hiding this comment.
Just to be sure. It may be superfluous but it doesn't hurt.
There was a problem hiding this comment.
Superfluous code can be very misleading
|
@mariushoch @filbertkm @tobijat Rage! This stupid bug keeps wasting my time. Merge! Also: WTF is wrong with Travis? |
|
Long story short: Neither Function.apply nor Function.bind exist in the JS engine used by phantomjs. There's a polyfill for Function.bind1 but that doesn't work here as Function.apply also doesn't exist (and I couldn't find a polyfill for that). I would suggest to completely remove this named functions code for now... the problems/ costs caused by that are just to huge (I just spent ages debugging phantomjs...). |
|
@mariushoch What now? I spent months debugging this issue. I'm now 100% sure this is the right way to do it. The tests pass in all browsers. Are you telling me we are disallowed to use JavaScript correctly because Travis uses a broken JS engine? |
|
I'm not sure how to move forward here... either kill the whole named function thing or allow the tests to fail until phantomjs gets fixed or we run the tests in another way, I guess. |
thiemowmde
changed the title
|
No longer needed as we decided to remove evilsSeed completely |
|
Erm, no? Why? Didn't I explained that in #45? |
thiemowmde
changed the title
thiemowmde
changed the title
There was a problem hiding this comment.
How about
namedFn = new Function( 'fn', 'return function ' + fnName +
'() { return fn.apply( this, arguments ); };' )( evilsSeed );?
There was a problem hiding this comment.
Yay for the 'fn' bit. Need to test this but looks good. Nay for the dropped bind. I think this is crucial.
There was a problem hiding this comment.
It was when I played around with my version. Since this is hard/impossible to test I'm afraid to change my version (the only one that works for sure, as far as I know) to something else.
This
ReferenceError: evilsSeed is not definedis driving me crazy. I can always reproduce it by simply shift+reloading a page in my local Wikibase Repo. This is a huge pain when debugging.Update: #45 removes this feature. It should be readded when the PhantomJS bug mentioned below got fixed and the Travis tests pass.