[Bug]: different behavior when calling link.attr with object vs string

[alexandernst]

What happened?

I have my command manager subscribed to stack:push events. Playing around with .attr() I found out that it will trigger a stack:push events depending on what parameters I pass to it.

Example:

const link = new dia.standard.Link();
const cm = new dia.CommandManager(...);

cm.on("stack:push", (commands, opt) => {
  console.log(commands);
});

link.attr("line/stroke", "red");  // <-- this WON'T trigger stack:push

link.attr({                       // <-- this WILL trigger stack:push
  line: {  stroke: "red" },  
});

I tried debugging the internals of Joint / Backbone and it seems that, by the time Backbone's .set() method is called, it's _pending flag has a different value. This then causes line 540 (backbone.js) to be called.

I'm not really sure if I'm doing something I'm not supposed to or if this is a bug.

Also, this seems to be happening only with Links.

Version

3.6.2

What browsers are you seeing the problem on?

Chrome

What operating system are you seeing the problem on?

Mac

[kumilingus]

I can't reproduce. Both commands triggerer stack:push (because they trigger change event on the link).

link.attr("line/stroke", "red"); 
link.attr({  line: {  stroke: "blue" }});

Note: the property value in the test case needs to be different otherwise no change event is triggered.

[alexandernst]

Hi @kumilingus ! I'm sorry, I should have made my homework by properly debugging the issue before submitting a ticket. I took my time to properly debug this unexpected behavior (hence the 2 days delayed reply), but I finally found what's going on!

It turns out that the options.propertyPath argument of the cmdBeforeAdd callback of the CommandManager is the culprit.

Consider the following demo:

const graph = new joint.dia.Graph();

new joint.dia.CommandManager({
  graph,

  cmdBeforeAdd: (cmdName, _cell, _graph, options) => {
    if (cmdName === "change:attrs") console.log(options.propertyPath);
  }
});

new joint.dia.Paper({
  el: $node,
  model: graph,
  width: 500,
  height: 200,
  gridSize: 10,
  interactive: true,
  preventContextMenu: false
});

var rect1 = new joint.shapes.standard.Rectangle();
rect1.addTo(graph).resize(50, 50).position(50, 50);

var rect2 = new joint.shapes.standard.Rectangle();
rect2.addTo(graph).resize(50, 50).position(250, 50);

console.clear();

var link = new joint.shapes.standard.Link();
link.source(rect1).target(rect2).addTo(graph);

link.attr("line/stroke", "orange");
link.attr({ line: { stroke: "red" } });

The first call to .attr() will cause attrs/line/stroke to be printed on the console, but the second call to .attr() will cause undefined to be printed.

The path isn't properly handled and passed to cmdBeforeAdd when an object is used when calling .attr().

[kumilingus]

It's not a bug. It's by design. What would be the propertyPath for the following expression:

link.attr({ line: { stroke: 'red', strokeWidth: 3 }});

[alexandernst]

IMHO it should be ["attrs/line/stroke", "attrs/line/strokeWidth"]. The rationale for this is that propertyPath provides (useful) information when .attr() is called with a string, but it doesn't provide any information at all when called with an object.

Since the propertyPath was designed to facilitate information to the subscriber of cmdBeforeAdd, the reasonable thing to asume would be that jj+ will try as hard as possible to facilitate that information, no matter if a string or an object was used when calling .attr().

A possible patch might look like this:

function flattenObj(obj, parent, res = {}) {
  for(let key in obj){
    let propName = parent ? parent + '/' + key : "attrs/" + key;
    if(typeof obj[key] == 'object'){
      flattenObj(obj[key], propName, res);
    } else {
      res[propName] = obj[key];
    }
  }
  return res;
}

flattenObj({ foo: [1, 2], line: { stroke: "red", strokeWidth: 8 } });
> {attrs/foo/0: 1, attrs/foo/1: 2, attrs/line/stroke: 'red', attrs/line/strokeWidth: 8}

Object.keys(flattenObj({ foo: [1, 2], line: { stroke: "red", strokeWidth: 8 } }));
> ["attrs/foo/0", "attrs/foo/1", "attrs/line/stroke", "attrs/line/strokeWidth"]

[kumilingus]

A few notes:

• IIt should be a new property called propertiesPaths.
• It's a little more complicated than you say because of rewrite: true option.
• It would slow down attr() calls (it's a low-level method that could affect the overall perfromance).

Perhaps we should send the original object via options. It's fast (we already have all the info).

link.attr({ line: { stroke: 'red', strokeWidth: 3 }});
// { propertyPath: 'attrs', propertyPathArray: ['attrs'], propertyValue:  { stroke: 'red', strokeWidth: 3 }}

And it's aligned with the current behavior of:

link1.attr('line', { stroke: 'white' });
// { propertyPath: 'attrs/line', propertyPathArray: ['attrs','line'], propertyValue:  { stroke: 'white' }}

Also, there is a simple workaround:

• always use attr() with path as the first parameter
• if you want to se multiple properties, use multiple attr() calls (you can even write a helper that use flattenObject and do the attr() calls in a batch)

[alexandernst]

Indeed, I completely forgot about { rewrite: true }! Passing the (original) object as propertiesPaths sounds like a reasonable way of implementing this without breaking anything and without affecting the performance👌 .

So, maybe the final signature of the callback can look like this:

cmdBeforeAdd(cmdName, cell, _graph, options = { propertyPath, propertyPaths, isRewrite })

The isRewrite could be a boolean that would allow the subscriber to check if .attrs() was called with { rewrite: true }. I'm not really sure we would actually need this if propertyPaths already contains the merged object.

[kumilingus]

It is not exactly what I meant. Please see this PR: https://github.com/clientIO/joint/pull/1946

Aftter this change, you should be able to build the propertiesPaths when you need them.

cmdBeforeAdd(cmdName, cell, _graph, options = { propertyPath, propertyPathArray, rewrite })

Note that rewrite option was already present when set to true.

[alexandernst]

I just checked the PR. This will be very useful without any doubt! Thank you for the fast patch! 🙏

[kumilingus]

Fixed in #1946 .