Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graphDiv masks hidden dependencies and complicates state management #355

Closed
vdh opened this issue Feb 23, 2018 · 3 comments
Closed

graphDiv masks hidden dependencies and complicates state management #355

vdh opened this issue Feb 23, 2018 · 3 comments

Comments

@vdh
Copy link

vdh commented Feb 23, 2018

@nicolaskruchten So to expand upon my concerns expressed in plotly/plotly.js#2389

react-plotly.js-editor has these special _fullData, _fullLayout, and _context properties as hidden dependencies drawn out from the graphDiv prop. I can't seem to find documentation on them, and the underscores seems to hint that they're supposed to be private internal variables to Plotly. It's implied that graphDiv is just supposed to be a wrapper for { data, layout }, but the source code and examples seem to say otherwise.

But since it's all hidden away inside this DOM Element wrapper, it's never fully explained in the docs that there's something special going on, and it's very unclear how to properly handle the separation of state management (like Redux, which should not be concerning itself with passing around a DOM element), and the behind-the-scenes integration between PlotlyEditor and the Plot / PlotComponent.

Maybe you'd prefer to always keep graphDiv as a required prop for the editor for the sake of tight integration, but data and layout (and any other relevant state information) should be separated out into dedicated props.

@nicolaskruchten
Copy link
Contributor

You're right that the editor depends on a number of _-prefixed keys that plotly.js adds to the DOM node. As we develop the editor, we are narrowing in on a clearer definition of just the keys that we need, but it's not yet clear to us, so we can't easily document it just yet, and for the time being we recommend passing around the whole node, much as it is unorthodox/ugly to store such an object in state/Redux/wherever.

I should add, however, that there is a subtlety here which I feel mitigates some of the distastefulness of this pattern: you do not really ever need to serialize/store the DOM node or its private keys, as these can all be regenerated from layout, data etc.

The simple example editor, you can see that the initial graphDiv is empty. This empty object is passed to both the PlotlyEditor and the Plot. The PlotlyEditor renders in an essentially empty state because it doesn't have access to the _ key. The Plot immediately creates them and updates the revision number such that the PlotlyEditor rerenders with the keys it needs. This pattern works just as well if the initial graphDiv is not empty and contains layout, data etc. This means that all the state you really need to serialize/store in an offline context is the non-_-prefixed keys, which are all documented.

@nicolaskruchten
Copy link
Contributor

FWIW this PR is an attempt to make clear what does and doesn't need to be carried around in an app's state: #384

@nicolaskruchten
Copy link
Contributor

The default export for this package now hides the handling of the DOM node and exposes only data and layout. Some of the non-idiomatic React stuff still happens under the hood but it's mostly abstracted away now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants