Props should not be mutated by Plot component

[swiperii]

It seems like the Plot component mutates its props (at least layout) instead of cloning the data.
Mutation of props is to me unexpected (and undesired) behavior for a react component.

If for instance the layout is stored in the state of a component and then fed to the Plot component as a prop, like

render() {
  <Plot
    layout={this.state.layout},
    ...
    />

we will have a problem, since this.state only should be updated with the react setState() method.

[nicolaskruchten]

Yes, this is a tricky situation, because plotly.js does mutate its inputs, whereas this is unexpected/discouraged behaviour in React. I'm not sure what the best way forward is here: if the React wrapper made a deep copy of layout before every update then Plotly.react's diffing logic wouldn't work, right @alexcjohnson ?

[alexcjohnson]

Tricky indeed. We've had some discussions about how to handle this but haven't settled on anything yet. It would be OK for the wrapper to deep copy layout and data (yes, data can get mutated too in some cases) before passing them on to Plotly.react except that you'd need to skip data_array items. The challenge then is that the copy of the plot state that you're holding doesn't match what's actually on the plot. Note also that there are some mutations that happen during the react/newPlot call, and these we don't report back in any way (ie by emitting an event), while mutations generated later via UI interactions are reported via restyle/relayout events.

I feel like the way forward is going to be something like:

• Add a config parameter to plotly.js telling us to manage data and layout immutably - some users depend on the existing behavior so we can't change the default until v2. But when that parameter is used:
• All changes to data/layout are done immutably, meaning gd.data and gd.layout become new objects as necessary.
• react/newPlot end by checking if these objects have changed (which at that point is just ===), and if so reporting them back (as a new event)?
• restyle/relayout/update also switch to immutable updates. I guess since these methods already emit events, we wouldn't necessarily need to emit the "mutation" event, though perhaps it would be easier if we did - so the react wrapper would only need to listen to the one event?
• On receiving this event, the react wrapper updates its own state. If this means that state gets passed back into Plotly.react that's fine - it means we'll diff again, find no changes, and do nothing more, so the only penalty is the diff itself. Actually, perhaps since the immutable config parameter is set, even the diff can be avoided by just seeing that data === gd.data and layout === gd.layout.

[nicolaskruchten]

The config parameter for immutable updates is just what I suggested to @etpinard this morning :)

That said, I think that the implementation suggest above will not resolve the main issue here unfortunately... Take a case where layout = {a: {b: 1}} and you want to set layout.a.b = 2. If you just change the identity of layout to a new object newLayout whose a key is a reference to layout.a then layout.a == newLayout.a and/but newLayout.a.b's value will have mutated, meaning that any code elsewhere which has grabbed a reference to a piece of layout can't use simple equality checks against the corresponding piece of newLayout. To avoid this you need the whole path from layout to any mutated key to become a new object for every mutation. This is basically what something like https://github.com/kolodny/immutability-helper does.

I must say that I also don't really understand the utility of subscribing to events and then calling Plotly.react again? The wrapper gets its state from upstream, but it's never read by anyone downstream, and it cannot push these changes back upstream (this goes against the whole React one-way-dataflow principle), so what is the downside to just calling Plotly.react and leaving it at that?

[nicolaskruchten]

On the other hand, while I know that this React component breaks React's single strict rule, I do want to inquire a bit about the actual concrete consequences today for users of the component... @swiperii where does this cause problems for your app? Are you relying on state comparisons elsewhere? My expectation would be that your app is still free to overwrite state whenever it likes and the component would update to match the new state etc. I'm not sure what the failure case is and I would love more information about it if you could provide some :)

[alexcjohnson]

To avoid this you need the whole path from layout to any mutated key to become a new object for every mutation. This is basically what something like https://github.com/kolodny/immutability-helper does.

Yes, that's what I had in mind. This kind of situation doesn't happen all that much, so I'm not worried about its overhead, but we would need to construct a new layout object all the way up the tree from the mutated value.

I must say that I also don't really understand the utility of subscribing to events and then calling Plotly.react again? The wrapper gets its state from upstream, but it's never read by anyone downstream, and it cannot push these changes back upstream (this goes against the whole React one-way-dataflow principle), so what is the downside to just calling Plotly.react and leaving it at that?

Right, I don't know what the right way of doing this is, but one way or another we need to be able to feed state back into the app from the plot. Certainly just closing this loop in the wrapper wouldn't solve the problem, but perhaps the wrapper can usefully mediate between the plot and the app? Other parts of the app could need to know when the user zooms in on the plot, for example.

I wasn't really thinking of subscribing to the event just to call Plotly.react again - I was just imagining that the plot state is changed, one way or another this needs to be pushed back into the app state, then the app passes it back down to the plot wrapper, so automatically Plotly.react gets called again. This part I'm also not worried about, because Plotly.react will quickly (and perhaps almost instantaneously since we have the immutable config arg) notice that it's seen this whole state before.

[nicolaskruchten]

OK, so I'm not sure why "one way or another we need to be able to feed state back into the app from the plot" ?

[nicolaskruchten]

Extra context: we already accept handlers for these events https://github.com/plotly/react-plotly.js/blob/master/src/factory.js#L10 ... but if Plotly.react mutates its inputs why would the upstream code need to know?

[nicolaskruchten]

PS: 👍 to the "all the way up the tree" approach to immutability... it wasn't clear from your initial comment if that's what you mean, so I'm glad we're on the same page!

[alexcjohnson]

Extra context: we already accept handlers for these events

Yeah, so maybe all I'm suggesting is a catch-all "state changed" handler so you don't have to worry about all these individual events, but also you get any changes made just by the process of drawing the plot.

if Plotly.react mutates its inputs why would the upstream code need to know?

I think these are all "you asked for auto-something, here's the something you got." I guess the main reason to want to pass these upstream is that update performance will be better if we don't have to recalculate those auto values (particularly autorange, and zmin/zmax for heatmaps and related types), but if they're not in the new state you pass in, we will have to recalculate them which triggers a slower update pathway. There may be a way for us to change plotly.js to avoid this problem, but that's a big project and possibly not even something we can do until v2, I'm not sure. Secondarily, other parts of your app may want to make use of these values. I guess for cases like this your app can look at _fullLayout and _fullData, but it could be even easier if they're already part of the app state.

[nicolaskruchten]

Ahhh I see. I'm always sensitive to performance-based reasons :)

Re the catch-all, the current wrapper does fire props.onUpdate() for the following plotly.js events: https://github.com/plotly/react-plotly.js/blob/master/src/factory.js#L37 ... Does Plotly.react fire those appropriately at the moment or are some changes required? Or do we need a new event type?

[alexcjohnson]

No, mutations that happen automatically during drawing do not generate any events right now. What would it take to make an event like this useful? Is it enough to just flip a flag somewhere and say "something changed during drawing, here's the final state," or is it important to know what changed?

[nicolaskruchten]

Probably just "here is the final state", because the instructions would basically be "feed this back to me please" :)

One thing to note here is that for versions of plotly.js before 1.34, this type of wiring would cause many updates to result in 2 newPlot calls :(

[swiperii]

@nicolaskruchten

You asked for how this caused a problem in our case, and while the state example I gave was a simple example of where things could go wrong when props are mutated, it perhaps did not describe a realistic scenario very well :)

The problem we have run into was a bit more complex. But basically we have a page with a list of objects, lets call them processes. When clicking on a process, the user is routed to another page displaying a plotly graph of the process status.

The data comes from a web API through the redux store, while the layout is a template object constant, that only sometimes is modified depending on the data.
Both are fed to a Wrapper component containing the plotly Plot component.
Going back from the plot page to the list page, the Wrapper and Plot components are unmounted and destroyed.

Now, going to another plot, using the same layout object (which originates higher up in the component tree, and thus not destroyed) we may suddenly have extra properties like selected tool or zoom range (depending on how the user interacted with the last plot) in our newly created plot that we did not expect.

Now that we know that props are mutated, it should be possible to work around until you have a longterm solution to what I understand is a complex problem. However, in the meantime, would it be possible to add this information to the documentation, e.g. in the Plotly component API?

[nicolaskruchten]

Thank you for sharing, this makes the problem quite clear. I apologize that our unclear documentation caused you undue issues in tracking this down and I'll see about making it clearer. In the short run, however, our goal is to actually fix this, obviously :)