Phantomjs fix

[jtmalinowski]

Hey,
Phantomjs is failing because apparently it (v1.9.2) doesn't have Function.prototype.bind.
The issue was noticed thanks to @tomelm, who was testing with Poltergeist (RoR gem) reactjs/react-rails#10

It can be reproduced easily if you have phantomjs:

page = require('webpage').create()
page.open 'http://facebook.github.io/react/index.html', (status) ->
  phantom.exit()

fails with:

TypeError: 'undefined' is not a function (evaluating 'RegExp.prototype.test.bind(/^(data|aria)-[a-z_][a-z\d_.\-]*$/)')

  http://facebook.github.io/react/js/react.min.js:18
  http://facebook.github.io/react/js/react.min.js:18 in r
  http://facebook.github.io/react/js/react.min.js:18
  http://facebook.github.io/react/js/react.min.js:19
  http://facebook.github.io/react/js/react.min.js:18 in r
  http://facebook.github.io/react/js/react.min.js:18
  http://facebook.github.io/react/js/react.min.js:18
  http://facebook.github.io/react/js/react.min.js:18 in r
  http://facebook.github.io/react/js/react.min.js:18
  http://facebook.github.io/react/js/react.min.js:20
  http://facebook.github.io/react/js/react.min.js:18
  http://facebook.github.io/react/js/react.min.js:20

and all subsequent errors are a result of this one.

After switching to custom binding function it appears to be working in phantomjs and poltergeist. All tests are passing. I don't think there is any reason for regression testing here.

I just noticed that same thing is happening to JSXTransformer.

[jtmalinowski]

I'm not sure this is the right branch, we would love to see this as 0.4.2 possibly.
And BTW, how come there are phantom tests and they have never shown this as a failure?

[benjamn]

I like this!

Couldn't you just do return /^(data|aria)-[a-z_][a-z\d_.\-]*$/.test(arg); in the new function?

[benjamn]

We also restrict the version of phantom we use to avoid problems with the latest versions. See package.js.

[jtmalinowski]

I'm still confused, I reproduced this using latest release (1.9.2) from http://phantomjs.org/download.html, but in package.json at 0.4-stable I have:

  "devDependencies": {
    ...
    "phantomjs": ">= 1.9.0",
    ...
  },

Liberal as it is, I think this would allow any latest version, including even major releases, perhaps there is another file, not package.json in the root of the project? I also couldn't find package.js, but I think that's a typo.

[tomelm]

I'm also running 1.9.2 when I'm seeing this error

[benjamn]

Ah yes, that line has changed a bit since 0.4-stable. Here's the latest version specifier: https://github.com/facebook/react/blob/658f41cb30/package.json#L52

It seems likely that, while the react tests passed when 0.4-stable was tagged, with what was then the latest version of phantomjs, running npm install on that revision today would fail because more recent versions of the phantomjs package have been released.

@zpao do we need a branch fix? I think this PR should do the trick.

[benjamn]

Nice, 👍

[zpao]

So here's the thing, we're going to keep using .bind if we need to. In fact we still use it in a few other places even with this fix. If we move React in the direction lots of other Facebook code is going, we're going to have other transforms which result in code using .bind.

The fact that Phantom doesn't support this is actually pretty concerning, especially for people using it to test UIs. In fact, it looks like Phantom is roughly equivalent to an early version of Safari 5.1, based on the examples/useragent.js they ship, it's webkit build 534.34, which is actually lower than Safari 5.1.0 (534.48.3). Assuming it uses the same JSC support as safari (reasonable assumption I think based on my perusal of webkit code), Function.prototype.bind was added in Safari 5.1.4. The best option for the world would be to upgrade Phantom to a version of webkit < 2 years old. Up to Safari 6 would be ideal (7 might be overshooting since it's not even out), but even to the latest in the 5.1.x branch would be better.

So the only other option really at that point for us becomes to build the Function.prototype.bind polyfill into the build, which I've said for a while that I really don't want to do. I think if you want to support a browser for your site that doesn't support a part of JS, that is entirely your choice, but for those that don't we shouldn't cram extra code in there. If you want to use phantom and depend on it, you need to make sure you support Safari 5.1.0.

@tomelm I think for your case this means, using es5-shim/sham and shipping it in your assets. Unless you don't want to support the browsers missing it.

[tomelm]

@zpao I wasn't concerned so much with the browsers that don't support as much as I can't seem to get any functional tests to pass that depend on React.

I'll see if maybe es5-shim/sham help with that thought.

[dillonforrest]

Hey guys! I found a hack. This hack got PhantomJS to keep running despite Function.prototype.bind.

Emphasis on "hack." This is not a real solution. This is just a hack, a workaround. Hopefully it'll be useful for some of you, or at least encourage some more meaningful conversation. :)

I added this code at the top of my react.js file:

if (!Function.prototype.bind) {
  Function.prototype.bind = function (oThis) {
    if (typeof this !== "function") {
      // closest thing possible to the ECMAScript 5 internal IsCallable function
      throw new TypeError("Function.prototype.bind - what is trying to be bound is not callable");
    }

    var aArgs = Array.prototype.slice.call(arguments, 1), 
        fToBind = this, 
        fNOP = function () {},
        fBound = function () {
          return fToBind.apply(this instanceof fNOP && oThis
                                 ? this
                                 : oThis,
                               aArgs.concat(Array.prototype.slice.call(arguments)));
        };

    fNOP.prototype = this.prototype;
    fBound.prototype = new fNOP();

    return fBound;
  };
}

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

Again, this is not a real solution. Hopefully just something so you can get back to the work which you actually wanna be doing. :) Disclaimer from source:

Some of the many differences (there may well be others, as this list does not seriously attempt to be exhaustive) between this algorithm and the specified algorithm are:

• The partial implementation relies Array.prototype.slice, Array.prototype.concat, Function.prototype.call and Function.prototype.apply, built-in methods to have their original values.
• The partial implementation creates functions that do not have immutable "poison pill" caller and arguments properties that throw a TypeError upon get, set, or deletion. (This could be added if the implementation supports Object.defineProperty, or partially implemented [without throw-on-delete behavior] if the implementation supports the defineGetter and defineSetter extensions.)
• The partial implementation creates functions that have a prototype property. (Proper bound functions have none.)
• The partial implementation creates bound functions whose length property does not agree with that mandated by ECMA-262: it creates functions with length 0, while a full implementation, depending on the length of the target function and the number of pre-specified arguments, may return a non-zero length.

If you choose to use this partial implementation, you must not rely on those cases where behavior deviates from ECMA-262, 5th edition! With some care, however (and perhaps with additional modification to suit specific needs), this partial implementation may be a reasonable bridge to the time when bind() is widely implemented according to the specification.

Anyway, I'd love to hear what you guys have to say about it. @zpao and team, thanks so much for the work on React!!

[benjamn]

@dillonforrest thanks for digging into this! We have a similar Function.prototype.bind polyfill that we shim during grunt test: https://github.com/facebook/react/blob/master/src/test/phantomjs-shims.js (required by https://github.com/facebook/react/blob/3dc1074908/src/test/all.js#L5).

If you're testing your own React components in PhantomJS, it's up to you to providing a similar polyfill. This pull request ended up getting closed because we don't want to avoid using .bind in the React core or bloat the core with polyfills just because of PhantomJS.

So I think the current recommendation is to pick your own fallback implementation of Function.prototype.bind if you need to use PhantomJS. That consistent with your views, @zpao?

[zpao]

@benjamn - 👍

@dillonforrest I'm glad you dug into it and understand the problem! My concerns above are still totally valid and have made me reconsider our usage of PhantomJS (updating Phantom would be a big task so switching to SlimerJS is looking pretty attractive). Anyway, glad you're enjoying React!

[zpao]

I just read https://groups.google.com/forum/#!topic/phantomjs/EiXb4iRU7WA and apparently PhantomJS 2 is on the horizon with an updated webkit, so this might all be temporary problem.

[dillonforrest]

@zpao I personally agree with your concerns regarding PhantomJS. It also puts friendly pressure on lagging software to catch up. :) And when lagging software improves, everyone is better off. Either way, I'm looking forward to PhantomJS 2!

[bitmage]

+1 on Phantom supporting ECMAScript 5. Right now it's like using IE as your test automation tool.

For those looking for alternatives, check out Selenium (wd for node), which uses actual browsers and also translates well to Sauce Labs CI testing, and zombie.js which is headless, so you don't get browser parity but it's very fast and supports ES5.