Upgrade to d3.js version 4 +

[daattali]

One piece of code that will need to be updated: d3.behavior.drag has been renamed to d3.drag

[cjyetman]

I have begun the process of converting to d3.js v4. I would like to be able to push my changes progressively, i.e. one graph type at a time. To facilitate that, I would like to send a PR (#158) that will add the current version of d3.min.js v4 (currently 4.4.1) to a directory that is parallel to the existing d3.min.js v3 directory. Because htmlwidget only loads specific files (or versions) listed in the appropriate YAML file, it should not cause a conflict to have both versions of d3 side by side, nor should it cause a problem with some graph types using d3 v3 while others use d3 v4.

If I am misunderstanding that or have missed some other potential conflict that will cause, please chime in. Otherwise, here’s the PR #158

[timelyportfolio]

@cjyetman thanks so much for starting the conversion. The only conflict that I foresee with this is combining widgets will have both d3v3 and d3v4, and since d3 is global, there will be problems. Perhaps, we could pursue an approach discussed in https://github.com/fbreitwieser/sankeyD3/issues/4 where we rename the d3v4 d3 as d3v4 or something other than d3, and then these can peacefully coexist (of course with lots of duplication). Also, we should be careful in the yaml to specify d3v4 instead of just d3 since the htmltools/htmlwidgets dependency management will choose the highest semver when it detects a conflict with dependencies with same name.

[cjyetman]

@timelyportfolio strange, as I understood it, htmlwidgets loads whichever version is explicitly stated in the YAML file for the given graph being plotted... is that not correct? It seems to be given the testing I have done.

Or is this a case of RStudio functioning as a glorified web browser and when you have multiple plots loaded in the viewer at the same time using different versions of D3, then every version of D3 that was needed is loaded into the global namespace?

[cjyetman]

@timelyportfolio does d3r help with this issue?

[timelyportfolio]

htmlwidgets are not self-contained, so if two htmlwidgets have name: "d3" in their yaml, or other htmltools::htmlDependency exists with name: "d3", then the highest version is chosen, which is generally a very nice feature except in this case with d3. You are correct that htmlwidgets will load the version stated in the yaml, so the problems only seep in when combining.

[cjyetman]

Man, that's a bummer. That's not a use case that I was testing nor all that familiar with (mixing different htmlwidgets in one shiny app or etc.). I could suggest aliasing d3 as soon as the widget loads like window.forceNetworkD3 = window.d3 but even in that case, the initial load would put the later version of d3 higher up on the preference list for the entire window. Maybe we could force the content of htmlwidgets into an iframe when it loads... probably feasible, but unfortunately unnecessarily complex. Is this being tracked in the htmlwidgets team? Seems like it would be best resolved there.

[timelyportfolio]

This is the most relevant conversation https://github.com/ramnathv/htmlwidgets/issues/121. It is tricky, and I don't think there is a good solution. It means that combining something like the d3v4-converted scatterD3 and a non-d3v4-converted htmlwidget like networkD3 will not work :(

[cjyetman]

I'm not opposed to modifying the v4 d3.min.js so that it loads as d3v4, and then adapting all the updated plotting Javascript so that it refers to things as d3v4.xxx and similarly modifying the updated YAML files with name: d3v4 so everything new works off a new name and doesn't conflict with anything old. That seems like a lot of non-standard stuff going on to keep track of though.

Or I could just work on my own fork and make a giant PR for updating everything all at once, which would also have to be combined with @fbreitwieser's already updated sankey code (#151) to work properly.

No perfect solution. Preferences?

[cjyetman]

Also, I understand there's concern for not causing conflicts internally amongst the all the types of networkD3 graphs. That's practically obvious. But are we also concerned about making sure that updated networkD3 graphs don't conflict with other htmlwidgets packages that are out there?

If so, then maybe doing the renaming is really the way to go. And even though it's a lot of "non-standard" stuff, it's stuff that would likely stick around for a while, and for good reason... until htmlwidgets comes up with a way to isolate things (through iframe or otherwise).

So maybe we should be working towards renaming D3 to networkD3-d3v3 and networkD3-d3v4 to avoid any internal or external conflicts?

[timelyportfolio]

One other option would be to build/rollup modules for each of the widgets in d3v4 form, so each exist independently and do not rely on the global d3. This of course results in duplication, but in many cases will have the advantage of being much smaller. The best reference is Mike Bostock (of course :)) https://bost.ocks.org/mike/d3-plugin/. The other advantage of this approach is that we can then give back these independent modules to the JavaScript community as reusable components.

[cjyetman]

As a result of this conversation, I'm gonna close PR #158 and rather push all changes for the update to D3v4 as one giant PR (eventually). At that time, there will still be a valid debate about what to do about causing conflicts with other, external htmlwidgets.

I believe that the best strategy is to move forward with updating to D3v4, using the same standard setup as has been used in the past and is used by most htmlwidgets, that is to declare in the YAML the name as d3 and use the legitimate version number for version. That will cause conflicts with external htmlwidgets that rely on D3v3 and use the standard setup, but not doing so will also ultimately lead to conflicts with external htmlwidgets that have updated. So either way, we will end up with conflicts.

In parallel to the update to D3v4, this discussion of workarounds and solutions is a good thing to be doing, so I created a new issue for that #162. Whatever solution is decided there can be applied regardless of whether we're currently using D3v3 or D3v4.

[cjyetman]

I have a functioning version of dendroNetwork() converted to D3v4 here... https://github.com/cjyetman/networkD3/tree/d3v4-dendroNetwork

This is the commit that implements the minimal conversion to D3v4: commit d3b7636

One can install this version of networkD3 for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-dendroNetwork')

My primary goal was to achieve the conversion to D3v4 with the absolute minimum changes. That means some things are still not ideal, but this should be the easiest path to a full package conversion to D3v4, and other improvements and changes can/will occur after that (based on my discussions with @christophergandrud).

In the interest of keeping all internal htmlwidgets on the same version, it was roughly decided above that we would push changes for all plot types at once, so I'm putting this here as a preview and as a way for people to give early feedback.

[cjyetman]

I have a functioning version of forceNetwork() converted to D3v4 here... https://github.com/cjyetman/networkD3/tree/d3v4-forceNetwork

This is the commit that implements the minimal conversion to D3v4: commit 5b29be1

One can install this version of networkD3 for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-forceNetwork')

My primary goal was to achieve the conversion to D3v4 with the absolute minimum changes. That means some things are still not ideal, but this should be the easiest path to a full package conversion to D3v4, and other improvements and changes can/will occur after that (based on my discussions with @christophergandrud).

In the interest of keeping all internal htmlwidgets on the same version, it was roughly decided above that we would push changes for all plot types at once, so I'm putting this here as a preview and as a way for people to give early feedback.

[cjyetman]

I have a functioning version of simpleNetwork() converted to D3v4 here... https://github.com/cjyetman/networkD3/tree/d3v4-simpleNetwork

This is the commit that implements the minimal conversion to D3v4: commit 425c5fc

One can install this version of networkD3 for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-simpleNetwork')

My primary goal was to achieve the conversion to D3v4 with the absolute minimum changes. That means some things are still not ideal, but this should be the easiest path to a full package conversion to D3v4, and other improvements and changes can/will occur after that (based on my discussions with @christophergandrud).

In the interest of keeping all internal htmlwidgets on the same version, it was roughly decided above that we would push changes for all plot types at once, so I'm putting this here as a preview and as a way for people to give early feedback.

[christophergandrud]

@cjyetman These look awesome! Thanks so much for putting them together.

In lieu of a an automated testing suite (as we discussed earlier), I've just been running the examples. They seem to work really well.

What still needs to be done to get these ready for merge into master?

[cjyetman]

well, I still have 3 of 6 to convert: chordNetwork(), diagonalNetwork(), and radialNetwork()

that is if we intend to stick to the plan of releasing them all at once, because otherwise we will need to include 2 versions of D3 and putting two of the internal htmlwidgets on the same page that use different versions of D3 will cause a fatal conflict

Otherwise, the above 3 are ready to go... though I would strongly prefer that a few people that know what they're doing run through a few examples and review my changes first.

[christophergandrud]

Sounds like a good plan to get them all out at once.

I'll run through all of the examples as you put them up. Would be great for others to have a go at testing different features to test it from as many angles as possible.

[cjyetman]

I have a functioning version of diagonalNetwork() converted to D3v4 here... https://github.com/cjyetman/networkD3/tree/d3v4-diagonalNetwork

This is the commit that implements the minimal conversion to D3v4: commit 1460fa3

One can install this version of networkD3 for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-diagonalNetwork')

My primary goal was to achieve the conversion to D3v4 with the absolute minimum changes. That means some things are still not ideal, but this should be the easiest path to a full package conversion to D3v4, and other improvements and changes can/will occur after that (based on my discussions with @christophergandrud).

In the interest of keeping all internal htmlwidgets on the same version, it was roughly decided above that we would push changes for all plot types at once, so I'm putting this here as a preview and as a way for people to give early feedback.

[cjyetman]

I have a functioning version of chordNetwork() converted to D3v4 here... https://github.com/cjyetman/networkD3/tree/d3v4-chordNetwork

This is the commit that implements the minimal conversion to D3v4: commit dd0d35a

One can install this version of networkD3 for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-chordNetwork')

My primary goal was to achieve the conversion to D3v4 with the absolute minimum changes. That means some things are still not ideal, but this should be the easiest path to a full package conversion to D3v4, and other improvements and changes can/will occur after that (based on my discussions with @christophergandrud).

In the interest of keeping all internal htmlwidgets on the same version, it was roughly decided above that we would push changes for all plot types at once, so I'm putting this here as a preview and as a way for people to give early feedback.

[cjyetman]

I have a functioning version of radialNetwork() converted to D3v4 here... https://github.com/cjyetman/networkD3/tree/d3v4-radialNetwork

This is the commit that implements the minimal conversion to D3v4: commit 508a28f

One can install this version of networkD3 for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-radialNetwork')

My primary goal was to achieve the conversion to D3v4 with the absolute minimum changes. That means some things are still not ideal, but this should be the easiest path to a full package conversion to D3v4, and other improvements and changes can/will occur after that (based on my discussions with @christophergandrud).

In the interest of keeping all internal htmlwidgets on the same version, it was roughly decided above that we would push changes for all plot types at once, so I'm putting this here as a preview and as a way for people to give early feedback.

[cjyetman]

@christophergandrud So I think that's it. I'm going under the assumption that all the other functions in the package utilize those 6 functions plus the sankeyNetwork() function, so having converted all the underlying JS files, everything in the package should work now with D3v4. There needs to be some testing, and then a merging of my commits above with a commit of @fbreitwieser's sankey conversion. I'll wait for direction on how to move forward with that.

[christophergandrud]

This is excellent! I'll try to carve out some time this week to do some testing. Would it be possible to merge your dev branches and @fbreitwieser's to test on (possibly) the final product?

[cjyetman]

I have compiled everything in this branch: https://github.com/cjyetman/networkD3/tree/d3v4-convert which can be installed for testing with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-convert')

But, this is NOT a 'final product'. Specifically, I just cherry picked files from @fbreitwieser's sankyD3 to get the sankyNetwork() function working here. However, the sanky plot throws errors on mouse events that I haven't fully investigated other than there are some jquery and shiny commands that were added in this commit to sankyD3 that cause the errors. It's roughly functional, but still has bugs.

There are much more than minimal changes for conversion to D3v4 in sankyD3, so we may have to get @fbreitwieser involved to sort things out... or I can start the conversion over from scratch and try to minimize the changes. Otherwise, it will take me a while to disentangle everything that has changed (e.g. there are new features, new function arguments for sankyNetwork(), etc.).

[cjyetman]

Since we haven't heard from @fbreitwieser, I went ahead and made a minimal conversion of the existing sankyNetwork() to D3v4. All minimal D3v4 conversion changes, as well as an upgrade to D3 v4.5.0, are now in this branch (d3v4-minimal_conversion).

install to test with...

devtools::install_github('cjyetman/networkD3', ref = 'd3v4-minimal_conversion')

@christophergandrud I've submitted all the changes in this branch in one PR #167

[cjyetman]

@christophergandrud should we close this issue now?

[christophergandrud]

Sure. I'll push the new version up to CRAN today.