Graph.setEdge() does not work in phantomjs when first argument is object

[artarf]

It seems that phantomjs has a bug in strict mode. Consider you have following code:

"use strict";
function f(x) {
  x = 1;
  return arguments[0]; // in phantomjs this returns 1, regardless of the given parameter
}
console.log(f({ attr: "foo" }));

Graph.setEdge() currently relies on strict mode working correctly. Suggested implementation would be something along these lines:

Graph.prototype.setEdge = function(v, w, value, name) {
  var valueSpecified = arguments.length > 2;

  if (_.isPlainObject(v)) {
    name = v.name;
    if (arguments.length === 2) {
      value = w;
      valueSpecified = true;
    }
    w = v.w;
    v = v.v;
  } else {
    if (!_.isUndefined(name)) {
      name = String(name);
    }
  }
  v = String(v);
  w = String(w);
  // ... rest is working correctly

@artarf thanks for pointing this out. I wasn't aware of the problem with PhantomJS and strict argument handling. I want to write a test for this to ensure that we don't get a regression down the line. I tried adding the following to test/browser-test.js which is run in Phantom by Karma, but I couldn't get it to repro.

  // This tests a problem in PhantomJS's handling of strict arguments
  it("can handle the object variant of setEdge", function() {
    var g = new graphlib.Graph();
    g.setEdge({ v: "v", w: "w" }, "value");
    expect(g.edge("v", "w")).equals("value");
  });

Do you see anything obvious I'm missing? I'd expect this to fail, but it passes.

[artarf]

I’ll take a look at this. First I’ll make a test that confirms the phantomjs bug. I could not find any mentions about it by googling. Maybe it’s just my configuration.

[artarf]

Hi Chris,

I could not confirm the bug in phantomjs. In a very simple setup, “use strict” functioned as expected.

The setup where this problem appeared was rather complicated. The thing I suspect most is heavy use of coffee script.

I’ll let you know if I find something worth of mentioning.

Thanks Arto. I don't mind making the change anyway for clarity's sake, though there is a small chance it might get inadvertently changed down the road without a failing test. I'll get it in this evening (~12 hours).

I have seen odd failures from PhantomJS in some of the projects that consume graphlib (e.g. dagre-d3), but haven't had a lot of luck tracking it down myself because it is hard to isolate at that level. The code was straight JS, so this may not be a coffee script issue. If you have any luck isolating your problem I'd appreciate it!

[artarf]

Actually I was using dagre-d3. I'll do my best to narrow it down.

[artarf]

Ok, the reason is now resolved. Phantomjs does not understand "use strict" if it is inside a function. I tried this with following coffee script code:

hello = (x) ->
  "use strict"
  x = "Finland"
  console.log "Hello, " + arguments[0] + "!" # this logs "Hello, Finland!"
  return
hello "World"
phantom.exit()

If "use strict" is moved to the top, then it correctly logs "Hello, World!".

dagre-d3.js combines several modules into single .js file and those are isolated from each other by enclosing them in functions. Thus "use strict" is always inside functions. I tested by adding "use strict" at the top of dagre-d3 and setEdge() worked as it should even in phantomjs.

The solution I suggested helps getting graphlib to work with dagre-d3/phantomjs, but this really is phantomjs bug.

[artarf]

Actually the bug occurs only when "use strict" is inside a function which is inside a function (coffee wraps the compiled code inside top-level "safety wrapper"). Good news is that PhantomJS 2 does not have this problem (nor coffee script support).

I pushed a change to address the strict issue. I verified by dropping "use strict" and then applying the change. If it works for you I will publish this in a new release of graphlib.

BTW, thanks for all of your work to track this down. It is much appreciated!

[artarf]

The fix is working, thank you.
With older version, I got test "bundle can serialize to json and back" to fail by changing bundling work like in dagre-d3. In browser.js:

- global.graphlib = require('./index');
+ module.exports = require("./index");

And in Makefile:

-       @$(BROWSERIFY) $< > $@ -x node_modules/lodash/dist/lodash.js
+       @$(BROWSERIFY) $< > $@ -x node_modules/lodash/dist/lodash.js \
+                       -s graphlib