PlotlyEditor handleUpdate mutates inputs instead of following unidirectional data flow

[vdh]

Due to the way the editor is wired to depend on the graphDiv DOM element from react-plotly.js, handleUpdate is mutating the data/layout, and then uses onUpdate as a "something changed" flag. So, it's essentially just bypassing the React render loop altogether by flowing some data mutations directly back to the embedded Plotly inside react-plotly.js.

What it should be doing is using something like immutability-helper to create an updated immutable copy of data/layout, and then send back those changes out as values to the onUpdate callback. The callback would then update the higher state, and those new changes would flow back down into the data & layout props of both the Plot and PlotlyEditor components.

[nicolaskruchten]

Your point is well-taken, and we do share your enthusiasm for the React/unidirectional dataflow pattern! That said, the architecture of plotly.js was laid down before the current vogue for immutable data structures, and so it's taking us some time to evolve things towards a state where this kind of thing is possible. The recent advent of the Plotly.react method is one part of this, and the proposed configuration flag for an immutable mode in plotly.js is another. In the meantime, this component does "cheat", as you've called it in another thread, and uses revision numbers as a fig-leaf.

I should note that it may not always be possible or desirable to make copies of everything, as some arrays in data or layout can be arbitrarily large, so there will likely always be this kind of revision-number escape hatch. If you have an array with a million entries and you want to repeatedly append one and rerender, it will not be performant to copy a million entries each time :)

[vdh]

@nicolaskruchten You're not going to have to "copy a million entries" if all you're doing is appending, even in an arbitrarily-large example like that. Immutability just means calling .slice() before that .push() and returning the duplicated array. You're only copying the container. It still contains all one million references to the original unmodified entries, plus one more. It may not be exactly the same performance due to the array clone, but please don't dismiss immutability out of hand like that on some untested performance boogeyman.

[nicolaskruchten]

I'm definitely not dismissing immutability, I'm working hard to promote it and we're making changes to plotly.js to use it! :)

That said, copying large arrays is much slower than "not exactly the same" ... Per https://jsperf.com/copy-large-array/1 for me the slice-and-push operation runs at ~60 ops/sec and the push-only runs at ~30,000,000 ops/sec.

[vdh]

@nicolaskruchten But that's just one part of the whole system necessary to actually fetch, render, and process user interaction events when dealing with data sets, even large ones.

Your performance test ignores:

• The render loop. e.g. DOM manipulations, and deciding which ones to do by diffing the data, mutated or not. Immutable can be shallow compared nice and quickly… but how long exactly does it take for mutated data to be diffed for rendering…?
• HTTP requests (where else is this large data set coming from, after all?)
• User interaction events (in the scenario that this data is generated or recalculated by the user)

Is there an actual real-world scenario where the time difference between immutable vs mutations actually makes an impact compared to all the much slower and more visibly-slow timing of fetching data and the render loop?

The only scenario I can think of where there's going to be many operations that quickly on a large ("millions") array of data is fetching real-time data via sockets. In which case, the network request turnaround and the render loop are both going to be much slower than any array manipulation code.

Performance tests and time-based optimisations are great, but prematurely optimising without taking the entire system into consideration will cause more problems than the optimisation solves.

[nicolaskruchten]

I agree with everything you're saying, and with the changes we're making to plotly.js you will be able to build applications which use immutability to do shallow equality checks and so on. I'm also saying that we'll leave a mutability/revision-number escape hatch in case there are others who want to do things differently :)

[nicolaskruchten]

Once #384 settles down, it will be easier to resolve this (and #212, which was already intended to do this!) as per https://github.com/plotly/react-chart-editor/pull/384#discussion_r172730087