onUpdate and onInitialized shouldn't pass a DOM element

[vdh]

Currently, the way the onUpdate and onInitialized callbacks are sending updates back up is by passing this.el, a.k.a. the graphDiv / gd.

This has created some awkward breakages of the React lifecycle because now instead of passing plain prop objects back up, there's a whole DOM element with hidden extra data smuggled aboard (_fullData, _fullLayout, the element ref itself, possibly more undocumented deps…?) being passed around instead.

The updated data and layout objects should be sent separately, either as individual callback params, or within a plain object wrapper. If the additional values (graphDiv, _fullData, _fullLayout, etc…) are still important dependencies that needs to be passed back up by the callback, then they should be broken out into their own unique params / wrapper keys. Then at least these advanced dependencies can be properly documented.

For example, react-plotly.js-editor has difficult state management because it's been coded to rely on the graphDiv exposed by the callbacks, instead of individual dependencies (data, layout, _fullData, _fullLayout, _context).

[nicolaskruchten]

Having thought about this a bit more, I'm not sure why returning a reference to a DOM node is a priori bad. Some clients of this code need to operate on this DOM node, and those who don't can easily grab whatever they want out of it, especially with destructuring assignment e.g. via onUpdate={ ({data, layout}) => etc }

The editor component can't operate without that entire node, including a large interconnected set of _attributes and _methods, and it's definitely not practical to break out all of these into individual arguments to a callback, so that components like the editor can stitch it all back together on the other end.

[vdh]

@nicolaskruchten Doesn't it concern you that you literally can't explain to me why you need all of the node? That it's just a huge undocumented kitchen sink of mystery secret sauce that we just have to grin and bear it? It's a big bundle of "magic".

But also again, it's a DOM node, it has absolutely no business being passed around as a prop, especially as a glorified container object. And because it's a DOM node instance, everything gets completely tied to that mutated instance that only works during runtime at that particular moment.

You can't reproduce any issues that are related to it, because it's all just instances referencing other instances that then get mutated somewhere secret. You can't serialise or unserialise it for testing or saving/loading state, because it's a whole DOM node that only exists during its creation by some other unrelated DOM manipulation code. It's a huge cheat that subverts the normal way props are used in React for unidirectional data flow.

I understand that graphDiv has become a bit of a sacred cow in the Plotly code base… but sweeping it under the rug and pretending it's an okay prop isn't going to solve the data flow problems it causes.

[nicolaskruchten]

I'm happy to explain why, I'm not sure where you get the idea that I literally can't :)

When plotly.js executes, it stores a number of intermediate values in various _attributes attached to the DOM node into which it was asked to render a plot. Subsequent calls to Plotly.restyle or the new Plotly.react method leverage some of these cached _attributes to be able to complete faster. plotly.js also attaches a number of functions like event emitters and callbacks related to animations. The editor component (not hosted in this repo, and hence a bit out of scope of this discussion) uses methods from plotly.js to query these cached _attributes on the DOM node it receives as input so as to be able to populate its UI with e.g. the right number of axes or whatever.

I'm also not sure why you say things can't be reproduced... We do it all the time in development: we can call Plotly.newPlot then Plotly.restyle over and over etc, and get totally reproducible results, as you can see all over our test suite. We work hard to ensure that the same data and layout inputs give the same plots, regardless of the details of the intermediate cached values of _fullLayout, _fullData etc. We ensure this via, among other mechanisms, image diffs.

Regarding serialization, one only needs to serialize data, layout and frames: everything else is derivable from those. If you look in something like _fullLayout it's basically a superset of layout with, perhaps, a few values mutated if the value of layout isn't internally consistent. A user has no need to serialize these _full* values because any version of plotly.js can regenerate them values it needs when first called.

Finally, you refer to the use of props but to be precise, in this repo, no DOM nodes are being used as props: they are being passed along by callbacks. I know you've objected to the use of DOM nodes as props in the editor repo, and the PR I merged this afternoon hid all this away so that users of the default exported API never need to see a DOM node, even though plotly.js and the editor are using it to communicate. It shouldn't cause any data-flow issues there unless you choose to reach into the state of the editor and serialize it.

I hope this helps :)

[nicolaskruchten]

Heads-up: #63 will go partway towards alleviating the issues you bring up. The DOM node will still be passed along, but as a second parameter to the callback. In the vast majority of cases, people will be able to ignore that parameter and just use the first one. This is technically a breaking change (even though I'll bet only the editor relies on the _attributes!) and will trigger a major version bump.

[vdh]

Thanks, this helps to clear things up better.

Previously I thought only data and layout were essential, it's good to find out that also frames and config are also important input props.

What I meant about reproducibility, is about being able to isolate an issue for debugging. You have to recreate the graph div via Plotly (and trigger the same changes) up to the moment before the issue happened on every debugging attempt, instead of the usual type of React debugging where you can just toggle between different props/state and re-trigger or step through the issue instantly as various components re-render. When everything's tied to an instance (instead of static prop data), that instance needs to be recreated continuously to be able to debug it. Additionally, the React dev tools will choke (Uncaught TypeError: Illegal invocation) when attempting to inspect DOM nodes.

Hopefully these changes will help to alleviate those debugging issues by making the graphDiv more of a cache prop than a data prop, so that the power of Plotly.react's prop diffing can make things easier.

[nicolaskruchten]

Certainly the documentation around data, layout, config and frames has been terse in this react-plotly.js repo and is a bit scattered in multiple places in the https://plot.ly/javascript/ documentation.

Now I see what you mean by reproducibility, which I would call "path-independence" in this context... Basically the notion that Plotly.react(x) or <Plot props={x} /> should yield the same output (which should be the same as Plotly.newPlot(x) regardless of how many times they've been called before with different values of x.

This is certainly the requirement we have with respect to Plotly.react() (and more generally with combinations of Plotly.newPlot()+Plotly.restyle() etc) but in practice we do encounter bugs where this is violated (e.g. #45 or #53) which we try to fix quickly. React itself, with its general-purpose VDOM and diffing etc is able to offer stronger guarantees of this behaviour than we are, but we work hard at it :)

Unfortunately, to set expectations: this PR will do little to change this particular state of affairs. You may still encounter path-dependent bugs like #45 or #53, regardless of whether or not you store the DOM node and its _attributes in a state container.

[nicolaskruchten]

Closing as #63 is merged, but I'm happy to keep the thread going if you have more questions.