Plotly.newPlot should be able to accept frozen objects

[rgbkrk]

Much to my surprise, after updating nteract to load initial notebooks as frozen values via Object.freeze (recursively), I noticed the newPlot mutates the data parameter.

Uncaught TypeError: Can't add property uid, object is not extensible

[rgbkrk]

I tried to "thaw" the object a tiny bit for Plotly with:

{
      data: Object.assign({}, figure.data),
      layout: figure.layout,
}

Now the plots are blank, yet there are no thrown errors.

[alexcjohnson]

@rgbkrk thanks! For the second part, if you can post reproducible code (or a codepen) for it we can investigate this as a bug.

For the first part - that's right, we actually mutate the input objects a fair amount - adding uids to traces seems to be the first instance but there are quite a few more. We've talked a fair amount (cc @etpinard ) about how this is not great practice but it will take a bunch of work if we decide to undo it.

Another approach would be to deep-copy these objects as they're provided to Plotly.plot/newPlot. I worry that it would be a lot of overhead for large plots. But it would also break another update method - which I'm not sure we've documented but I know some people have used: modify the objects you passed in and call Plotly.redraw(graphDiv). If users modify the copy of the object attached to the plot div it would still work, but if they hold onto a copy of the object they initially plotted and modify that it would break.

Modifying a plot through the methods restyle, relayout, addTraces and so on also mutates the internal data and layout. This is probably always going to be the case, particularly due to streaming plots, where you have a large data array and want to frequently add one or two points to the end of it. But I suppose if we were to go the deep-copy route that could be just mutating our internal state, rather than the objects the user provided.

Anyway, this is definitely a discussion we should have, and all of this is up for grabs in v2.0.

One thought I had - that I don't think is a good idea but I'll mention it just for completeness - is that we could check if any input object is frozen (Object.isFrozen) and deep copy them if so. That might solve your immediate issue but seems to invite confusion, as people would try freezing, see that we don't mutate the objects, and then expect that to always be the case.

[rgbkrk]

I definitely understand mutating these in place, especially for large traces. As long as I know that data I pass in gets mutated, that's ok. The way that I'm using data with plotly (as well as our other views) is in a manner that can be synchronized across multiple clients. One way of enforcing this during development is pure functions and immutability. As long as I know the best practice for me to setup a new plot (99% of the case), then we're golden.

Checking if an object is frozen and either deep copying them or throwing an explicit error would be helpful. I think it's ok for the library to bail with a good error message (so developers see it early on in their development cycle).

[gvwilson]

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson