Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vis Network + vue.js: When adding a new edge - all nodes become red and edges overlap #2567

Closed
FilipQL opened this issue Jan 11, 2017 · 17 comments

Comments

Projects
None yet
9 participants
@FilipQL
Copy link

commented Jan 11, 2017

Hi,

In which case Vis changes the color of nodes to red?

I have created a very simple app using Vue.js & Vis Network. You can take a look at it here: https://jsfiddle.net/Filip_Z/2ewtzh7w/1/
For some reason, when I add a new edge - all nodes become red and every newly added edge will overlap with the existing one:

1

Now, here is the same version of this app without Vue: https://jsfiddle.net/Filip_Z/63jcj26y/2/

... and there is no this problem. But, the problem for me is that there are no errors in the browser console and I cannot figure out why is this happening. Do you know in what case Vis changes the color of nodes to red (and every newly added edge will overlap with the existing ones), in which cases Vis behaves like that? I'm asking because I don't know how to debug/solve this, I have no idea what is causing this behavior.

@Tooa

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

There was a similar problem with the dataset where all nodes got the title 4 after an update here? Looks like visjs does not work properly with vue.js or something is missing on the vue.js side in order to work properly with visjs.

This issue is really strange. However, I think it is easier to debug the setData method mentioned in the other issue. Looks like they are related.

@Tooa Tooa changed the title Vis Network: When adding a new edge - all nodes become red and edges overlap Vis Network + vue.js: When adding a new edge - all nodes become red and edges overlap Jan 12, 2017

@FilipQL

This comment has been minimized.

Copy link
Author

commented Jan 15, 2017

With Vis version 4.10.0 nodes don't turn red but the edges still overlap.

@Tooa Tooa added this to the Minor Release v4.19 milestone Jan 15, 2017

@mojoaxel

This comment has been minimized.

Copy link
Member

commented Jan 15, 2017

@FilipQL It looks like it works with v4.18.0: https://jsfiddle.net/k5z587tv/

@Tooa

This comment has been minimized.

Copy link
Member

commented Jan 15, 2017

@mojoaxel doesn't work for me with your Fsfiddle. Once I attach an edge to a created node, all nodes become red.

@FilipQL

This comment has been minimized.

Copy link
Author

commented Jan 15, 2017

@mojoaxel It doesn't work for me.

@Tooa I'm not sure where is the problem because without Vue - everything works fine. I have started a new thread on the Vue Forum: https://forum.vuejs.org/t/vis-js-does-not-work-properly-with-vue-when-adding-a-new-edge-all-nodes-become-red-and-edges-overlap/5132 and on Laracasts: https://laracasts.com/discuss/channels/vue/visjs-does-not-work-properly-with-vue but there's no response for now. I'll write here when there is any interesting response, if I find out what's the problem.

@mojoaxel

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

@FilipQL @Tooa Sorry, my mistake. I can confirm it now.

@Tooa Tooa removed this from the Minor Release v4.19 milestone Jan 23, 2017

@mojoaxel mojoaxel modified the milestone: Major Release v5 Feb 16, 2017

@pmackay

This comment has been minimized.

Copy link

commented Mar 15, 2017

I'm also working on an app that uses Vue and Vis Network and have reproduced this. I've found that it only happens if the vis.Network object is stored in the data hash on the Vue component. So far it seems ok if the vis object is attached to the window and the nodes, edges and options objects are stored in the Vue data hash.

Any suggestions of why this difference might affect the main Network object are appreciated!

@gustavHsiung

This comment has been minimized.

Copy link

commented Apr 21, 2017

Thank you @pmackay, it works!

@DanielWLam

This comment has been minimized.

Copy link

commented Jun 7, 2017

@pmackay it does work, however, the code seems not so elegant in my aspect

@pmackay

This comment has been minimized.

Copy link

commented Jun 7, 2017

@DanielWLam I agree its not elegant, a better way would be to fix this bug :) But its a rather odd bug...

@wimrijnders

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2017

Analysis of This Issue

So....I spent a whole morning looking at this. Please accept my nomination for 'Hardest Bug to Find, Ever'. If this category does not exist, please establish it so I can win it.

I'll need to describe the scenario first:

1. vue.js adds proxies to all member variables of all objects.

The initialization of vue.js looks something like this (taken from example):

Vue.component('ovs-network', {
    template: "#mytmp",
    data() {
        return { /* Some object containing state */ };
    },

    computed: {
        graph_data() {
          return { /* Some object containing state, can use stuff init'ed in data */ };
        }
    },

    mounted() {
        /* Do initialization here, can use stuff from data() and mounted */
    }

The fields of the object returned by data() is added directly to this; same goes for the object defined at mounted. Hence the result of graph_data() can be accessed as: this.graph_data.

However, the object in question that are passed are not used as is; they are converted to new objects with proxies for every member. This adding of proxies recurses into contained objects and arrays.

As an example, since we're dealing with the background color here, in the default options, the color is defined as:

      color: {
        border: '#2B7CE9',
        background: '#97C2FC',
        highlight: {
          border: '#2B7CE9',
          background: '#D2E5FF'
        },
        hover: {
          border: '#2B7CE9',
          background: '#D2E5FF'
        }
      }

This is just a simple object. After vue.js deals with it, it becomes (from chromium console):

post_proxy

Please read up on getters and setters to understand what this means. I'm not an expert myself, I'm just going by intuition here as to what they do.

2. Module network of vis.js uses prototype chaining for the options.

Rather than copying all options for every item in a network, network set progressively more global options as prototypes for more specific options. For example, for a given node, the chain would be:

node options -> group options -> application options -> default options

...for every node in a network (edges work comparably).

In the case of the example, there are no group options, so these are left out. The application-specific nodes options only has physics, which is not relevant here and thus will ignore. The options for the nodes in the example list would look like this:

+---------+
| id: 0   |
| ...     |
+---------+
+ proto   +--------->+
+---------+          |
                     |
+---------+          |
| id: 1   |          |
| ...     |          |
+---------+          |
+ proto   +--------->+
+---------+          |
                     |
+---------+          |                 +-----------------------+
| id: 2   |          +---------------->| NodesHandler defaults |
| ...     |          |                 | ...                   |
+---------+          |                 | color: {}             |
+ proto   +--------->+                 | ...                   |
+---------+          |                 +-----------------------+
                     |
+---------+          |
| id: 3   |          |
| ...     |          |
+---------+          |
+ proto   +--------->+
+---------+          |
                     |
+---------+          |
| id: 4   |          |
| ...     |          |
+---------+          |
+ proto   +--------->+
+---------+

Continued in following comment

@wimrijnders

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2017

This continues the text in the previous comment

3. Creation of options without vue.js

In the usual case, when a node is created, an options object is created for that node. This object is local with the prototype set to a global options object for all nodes[1].

untitled1

When shown property background is set in this object, it puts this in a local variable; the value in the prototyped object is untouched.

E.g. doing this:

object.background = '#222';

Leads to this:

untitled2

4. Creation of options with vue.js

When using vis.js in conjunction with vue.js, as in the previous comment, the default options object has proxied properties. On node creation, the associated options object looks like this:

untitled3

In this case, setting a value for background defers to the prototype object. Apparently, the interpreter looks up the prototype chain, finds the setter for background and uses it. The result is that background is set in the prototype object:

untitled4

As implied by the diagram, the prototype object is shared by all nodes. Hence, the result is that the given property is changed for all other nodes as well.


[1]: In the code, done with util.bridgeObject(referenceObject): creates an empty object with prototype set to referenceObject.

@wimrijnders

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2017

Continues previous comment.

5. Explanation of the Issue

This is what is happening here:

  • Manipulation is enabled; 'Add Edge' is selected and the cursor is dragged away from inside a node.
  • This causes a controlNode to be created. It's the little bright red circle at the end of an unconnected node:

untitled

  • The background color for this node is #FF0000. On initialization, 3. (above) occurs.
  • Since all nodes share the prototype object, all node colors change.
  • Note that this can happen on all options variables. E.g. in #3142 the same thing happens with label.

6. Resolution of this issue

a. The most practical solution for this is to move the options out of the data() method of vue.js, as previously indicated by @pmackay above. This prevents the proxying of all option variables. Preferably, the contents of graph_data() should be moved out as well.

This, of course, mutes the whole point of using vue.js in the first place. I'm really wondering the following, for which I hope to get an answer:

Is there a use case for having all variables within a vis.Network instance reactive? Perhaps just some of them?

Please let me know, because I really do not see the point.

b. An alternative is to adjust vis.network so that it does not use prototype chaining any more. All options are collapsed into a single object, as discussed in 'possibility 2' of #3060.

I have the following objections on this:

  • Prototype chaining is a perfectly valid construction in javascript. That vis.js is using it is not the issue, rather it's that vue.js can't handle it.
  • It would mean that every node (and edge) would have its own complete set of options. This is unnecessary memory overhead IMHO, especially for large networks. Network with over 20.000 nodes are not unheard of, see #2741.

c. The way to get vis.js as is working nicely with vue.js, is to abide by the rules that vue.js lays out with respect to reactive variables. See the quote in #2524.

This entails specifically tailored code adjustments in vis.js; I am not going there and neither are you.

d. Beyond this, I have no clue what else can be done. I'm really hoping for some external input on this. If I think of anything, I'll add it here.


This concludes my analysis for this issue. Is there reason enough to keep it open?

@wimrijnders

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2017

Feedback from a vue.js maintainer:

@wimrijnders I looked at your issue and your analysis is correct - in short, it is not recommended to put complex objects with prototypal inheritance into Vue's data option (and making them reactive). Moving them out of data is the correct solution.

So, moving the options out of data() is the recommended thing to do. Can we close this (and related) issue(s) now?

@wimrijnders wimrijnders marked this as a duplicate of #2524 Jul 16, 2017

@mosinve

This comment has been minimized.

Copy link

commented Jul 17, 2017

As for me, if options object is set once before network init, it has to be in a single module and exported as const (using ES6)

@wimrijnders

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

Closing because I feel this issue has been amply addressed. Will server as a reference for future similar issues.

@morank92

This comment has been minimized.

Copy link

commented Dec 8, 2017

终于解决了,非常感谢,开心

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.