ACE-IoT-Solutions / ace-svg-react

Grafana SVG Rendering Panel using the 7+ Plugin Framework
https://grafana.com/grafana/plugins/aceiot-svg-panel/
MIT License
40 stars 14 forks source link

Attempting to create elements and reference them between init and render causes circular references in the panel json #23

Closed acedrew closed 2 years ago

acedrew commented 2 years ago

Fix is to move transient state to a new context object that's passed between the init and render functions

wz2b commented 2 years ago

The problem here is that if you try to store an SVG.js node in options it causes a circular reference error that makes it so that Grafana can no longer serialize the panel settings. When this happens you can't save the panel any more, nor can you view the panel JSON.

One idea to fix this was to add a separate 'context' or 'state' parameter that would be passed into the user's panel initialization script as an empty object. The user could then add whatever they wanted to this object, and it would in turn be passed to subsequent invocations of the rendering script.

This would work, but may be unnecessary. As @acedrew pointed out to me (in a sidebar), you can also hang your arbitrary state off of svgnode, as long as you don't interfere with svgnode. That means not naming your additions dom, events, node, or type ... but because svgnode is a more deeply nested object than that, you also have to avoid any names by anything else in the SVG.js hierarchy of objects.

Options:

  1. We can just change the docs and instruct people to have their data off of svgnode
svgnode.state = {
     a = 1, o = someObj
}

then add that to the docs and change the example

  1. We can go ahead and add a separate 'state' or 'context' parameter, as the initial ticket proposes, then just be very careful to store it a place where grafana itself won't see or mess with it.

I am leaning toward option 2, and naming it 'context' rather than 'state', even though it's slightly more work than option 2. I think it's a little safer, because it eliminates the chance of someone doing something in their script that stomps on svgnode.

acedrew commented 2 years ago

Thinking more about this, blowing up peoples dashboard json is a really bad thing to do, I wonder if there's a way to check for circular references in options prior to handing them off to grafana and just dropping any root node in the options that points to a circular relationship @evannorton Can you investigate this? @wz2b is working on a PR for the context object, but I'd like to try to protect the users from a footgun here too.