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

EdgesHandler._addMissingEdges Cannot read property 'forEach' of null #3562

Open
Stexxen opened this Issue Oct 12, 2017 · 27 comments

Comments

Projects
None yet
@Stexxen
Copy link
Contributor

Stexxen commented Oct 12, 2017

On upgrading from 4.20.1 to 4.21. I'm now seeing this error in the console.
image

This appears to be caused by #3516 and discussed in #3500

Exception is raised at this line - https://github.com/almende/vis/blob/master/lib/network/modules/EdgesHandler.js#L468

Can someone explain if these changes are meant to cause this error and does it require different data structures now?

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 12, 2017

Aaaand the first issue for release 4.21.0 is in!

No, that's not supposed to happen. The data structure is exactly the same as previous version.

But this means that there is no DataSet instance for edges defined....is it remotely possible that you do not pass any edge data in?

In any case, this will need a fix.

@wimrijnders wimrijnders reopened this Oct 12, 2017

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 12, 2017

Oops sorry wrong button.

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 12, 2017

I'm trying to reproduce this through the standard API without much success. Could you give me some example code that shows this problem?

@Stexxen

This comment has been minimized.

Copy link
Contributor Author

Stexxen commented Oct 13, 2017

will do.

@Stexxen

This comment has been minimized.

Copy link
Contributor Author

Stexxen commented Oct 13, 2017

Tracked it down. (Well how to cause it anyways.....)

Here's an example using the Getting Started Code
https://plnkr.co/edit/fczlRG6U9El1rd3xLFx6?p=preview

The problem appears to be related to setting an options field.

var options = {
  "nodes":{
    "physics":true
  }
};

triggers the bug, but the weird thing is that it doesn't matter what the value is

var options = {
  "nodes":{
    "physics":false
  }
};

also triggers the bug

Whereas

var options = {
  "nodes":{
    // "physics":true
  }
};

Does not.

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

OK, got it. Can reproduce. Thanks for your sleuthing!

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

That's a nice interplay of internal 'features'. The options get handled before the data and physics option triggers a dataChanged event. But there is no data at that point.

All this can be solved with a strategically placed test on null. But I'll delve a bit deeper to see if there are further consequences. Bet you a beer icon same thing happens with edges.physics.

@wimrijnders wimrijnders added Confirmed Bug and removed Problem labels Oct 13, 2017

@Stexxen

This comment has been minimized.

Copy link
Contributor Author

Stexxen commented Oct 13, 2017

Yep edges.physics triggers it as well.

🍺

:-)

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

Yay! One for the collection.

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

Was introduced in this version; had to check. We're planning a quick release in 2 weeks time precisely for this kind of shiissues. Thanks for reporting!

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

This is where the dataChanged is triggered:

if (options.hidden !== undefined || options.physics !== undefined) {
this.body.emitter.emit('_dataChanged');
}

Let's try options.hidden as well, shall we?

explosion3

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

Best thing to do is to block handling of dataChanged as early as possible if there is no data. I'm on it.

wimrijnders added a commit to wimrijnders/vis that referenced this issue Oct 13, 2017

Network: Prevent crash when dataChanged is triggered during initial s…
…etting of options

Fixes almende#3562.

Options `hidden` and `physics` can emit a `_dataChanged` event within `setOptions()`.
If this happens when setting options during the initialization of the `Network` instance, this leads
to an error thrown, because there is no DataSet instance  connected yet to the instance.

This bug was introduced in `v4.21.0`. Unit tests have been added for this case.
@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

Wrt to your comment on the PR: it doesn't matter what you fill in for physics, the error will be triggered. So 'false' is as good as any. You noticed this yourself.

@Stexxen

This comment has been minimized.

Copy link
Contributor Author

Stexxen commented Oct 13, 2017

yeah sorry I deleted it as soon as I wrote it. doh!

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

No worries. Here, have a beer (icon): 🍺

yotamberk added a commit that referenced this issue Oct 13, 2017

Network: Prevent crash when dataChanged is triggered during initializ…
…ation (#3568)

* Network: Prevent crash when dataChanged is triggered during initial setting of options

Fixes #3562.

Options `hidden` and `physics` can emit a `_dataChanged` event within `setOptions()`.
If this happens when setting options during the initialization of the `Network` instance, this leads
to an error thrown, because there is no DataSet instance  connected yet to the instance.

This bug was introduced in `v4.21.0`. Unit tests have been added for this case.

* Edited comment
@justinharrell

This comment has been minimized.

Copy link
Contributor

justinharrell commented Oct 13, 2017

Sorry about this one, should have known better than skip a null check.

@wimrijnders

This comment has been minimized.

Copy link
Contributor

wimrijnders commented Oct 13, 2017

@justinharrell not a problem, shit happens. It wasn't obvious at all that this could happen. In any case, it's shared guilt, since I could have noted it when reviewing.

I'm just glad we got this so early (thanks OP), in two weeks time with the next release it'll be history.

@dakk

This comment has been minimized.

Copy link

dakk commented Oct 19, 2017

I'm still getting the error using a DataView instead of using an array or DataSet; the problem is not a null data but a missing forEach on DataView. I expected that DataView inherits forEach from DataSet, but the forEach method is not present on DataView.

I think we need to put something like:

// copy foreach functionality from DataSet
DataView.prototype.forEach = DataSet.prototype.forEach;

here: https://github.com/almende/vis/blob/master/lib/DataView.js#L391

@justinharrell

This comment has been minimized.

Copy link
Contributor

justinharrell commented Oct 21, 2017

Oops again, thought I checked and made sure Dataview had forEach. Definitely makes sense to me that it should.

@dakk

This comment has been minimized.

Copy link

dakk commented Oct 22, 2017

Is it necessary a new issue @justinharrell ? This was related to a null reference, while my report is about missing method

watho added a commit to watho/visjs that referenced this issue Dec 22, 2017

@dhermanns

This comment has been minimized.

Copy link

dhermanns commented Jan 26, 2018

Hi! Any idea when the fix will be released?

@romak1984

This comment has been minimized.

Copy link

romak1984 commented Feb 20, 2018

Any progress with this?

@aruljane

This comment has been minimized.

Copy link

aruljane commented Feb 25, 2018

Any progress guys?

@aruljane

This comment has been minimized.

Copy link

aruljane commented Feb 25, 2018

Guys, I tried a work around to disable physics and it worked,

`
var data = {
nodes: nodes,
edges: edges
};

    var options = {
        nodes: {
            shape: 'circle'
        },
        edges: {
            arrows: 'to'
        },
        physics: false
    };
    var network = new vis.Network(container, data, options);

`

@amarvin

This comment has been minimized.

Copy link

amarvin commented Apr 13, 2018

My workaround is to keep using the older Vis.js version 4.20.1

@burgalon

This comment has been minimized.

Copy link

burgalon commented Jul 31, 2018

Thanks @amarvin

@JanTheHun

This comment has been minimized.

Copy link

JanTheHun commented Aug 29, 2018

The bug still exists.

@aruljane-s workaround doesn't seem to work for me. data is definitely not null.

Any suggestions? This bug is almost a year old, is it possible to fix it somehow?

EDIT:
sorry, my mistake, @aruljane-s solution (moving physics field one level up) DOES work!

EDIT2:
maybe the documentation should be updated?

burgalon added a commit to burgalon/react-graph-vis that referenced this issue Sep 5, 2018

@mojoaxel mojoaxel added this to the Minor Release v4.22 milestone Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment