Use upstream d3-sankey and d3-sankey-circular

[adamreeve]

Plotly.js currently uses a forked version of d3-sankey: https://github.com/plotly/d3-sankey

This appears to have been originally forked due to the upstream maintainer not being responsive to a PR: d3/d3-sankey#19 (comment)

However, the Plotly fork is now out of date with upstream d3-sankey and missing some features, notably the ability to control the order of nodes, which has been requested in #4373.

The Plotly fork could be updated, but it appears that all the features that were added to the fork have now been added upstream or reverted, so it might be simplest to switch to using the upstream d3-sankey library directly.

Fork features:

• Stable sort ordering
  • Added to the fork in Stable sorting links for better parallel edge handling d3-sankey#1
  • Merged upstream in Stable sorting links for better parallel edge handling d3/d3-sankey#19
• Filled link paths
  • Added to the fork in Filled link path d3-sankey#2
  • PR made upstream was never merged (Filled link path d3/d3-sankey#24) and this is now out of date as upstream has switched to using d3-shape for the links (d3/d3-sankey@0dc1f11).
  • This feature was dropped from the fork as part of update to d3-sankey 0.7.1 + fix problem with large padding d3-sankey#7
• Padded space fix
  • Added to the fork in ensures that padded space is not larger than figure d3-sankey#3
  • Upstream PR: Fix for large padding d3/d3-sankey#63
  • Fixed upstream in d3/d3-sankey@0fa75d8

plotly.js similarly uses a forked version of d3-sankey-cirular: https://github.com/plotly/d3-sankey-circular. This has been archived and is also behind its upstream library, which also has support for setting nodeSort.

Do you have any thoughts on this @monfera, @antoinerg or @etpinard?

[camdecoster]

Thanks for investigating this. Have you tried swapping the libraries out? If so, did things just "work"? I'm trying to understand how much work will be involved in making this change.

[adamreeve]

Yes I tested this quite a while ago and also got the nodeSort feature working, and I seem to remember that everything just worked. I will make a PR with this and see what happens.

[adamreeve]

I've opened a draft PR at #7696. This seemed to mostly work but there's a regression with the large padding test that will be a problem: https://github.com/plotly/plotly.js/pull/7696/files#r2715057996.

I guess the upstream fix in d3/d3-sankey@0fa75d8 didn't actually address the problem correctly, or maybe this regressed in more recent versions.