Skip to content

Conversation

emilhe
Copy link
Contributor

@emilhe emilhe commented Feb 10, 2022

Officially, Dash only supports passing other Dash components via the children prop. And for that reason (I guess), a check is being performed in the Python layer (in base_component.py) to catch if people try to do it,

if k != "children" and isinstance(v, Component):
    raise TypeError(...)

However, it is possible to render components passed via non-children properties - in fact I have developed some JS code (available in dash-extensions-js) that makes it possible to do it in a single line of code. This option dramatiacally improves the development experience and the flexibility of the resulting Dash components allowing stuff like,

import dash
import dash_mantine_components as dmc
from dash_iconify import DashIconify as Iconify

app = dash.Dash()
app.layout = dmc.Button(
    "Send Mail", leftIcon=[Iconify(icon="fluent:folder-mail-16-filled")]  # dash component passed as non-children property
)

if __name__ == "__main__":
    app.run_server()

image

This code already works with the current version of dash, but only because the list wrapping of the Iconify component cheats the check listed above. Hence the following code will not work with the current version of Dash,

import dash
import dash_mantine_components as dmc
from dash_iconify import DashIconify as Iconify

app = dash.Dash()
app.layout = dmc.Button(
    "Send Mail", leftIcon=Iconify(icon="fluent:folder-mail-16-filled")  # dash component passed as non-children property
)

if __name__ == "__main__":
    app.run_server()

since the check will be hit. In this PR, I am proposing to remove the check so that the above code will work too.

The main limitation of this approach is that components rendered this way are now known by the dash renderer, so they cannot be targeted by callbacks.

@emilhe emilhe requested a review from alexcjohnson as a code owner February 10, 2022 21:24
@alexcjohnson
Copy link
Collaborator

@emilhe really interesting solution with dash-extensions-js! This issue has been on our radar for a long time - the only mention I can find of it in a quick search is #1722 but we certainly discussed it internally long before that.

A big caveat with this method is that the dash renderer doesn't know that these components exist, so they can't be used in callbacks as either inputs or outputs - we get that info by crawling the layout looking for IDs, but we only descend into children, no other props. That's fine for your use case here, but app devs could easily get tripped up if they wanted to for example change the icon, they'd have to change the whole leftIcon prop, not just the icon prop inside the Iconify component.

It's also a little worrisome that dash-extensions-js duplicates a bunch of the logic from dash-renderer - this part of the renderer has been pretty stable, but if it ever did change in the future that could break components using this.

On the other hand, renderDashComponents being imperative has a nice side effect that you can call it selectively. One use case for components inside arbitrary props would be richer content in dataTable, but if we always needed to crawl all the way into the data prop looking for components to render, that would have a huge performance cost. The way you've implemented this it could be enabled only when you're making a small table where the cost is acceptable, or only for the cells that actually get rendered.

So I'd love to see if we can find a way to accomplish this, but keep the cool ability to selectively render but do it off the dash-renderer codebase, and maintain the ability to link these components to callbacks.

@emilhe
Copy link
Contributor Author

emilhe commented Feb 11, 2022

@alexjohnson yes, I have looked through (some of) the discussions. The performance considerations mentioned there was one of the main reasons I decided to work on an solution via external library rather than pursuing a PR.

I am (painfully) aware of the callback limitation. I actually have a lot of use cases where this limitation is a show stopper, but I figured that with an external library this was probably 'the best that could be done' (to enable callbacks, I guess the solution would have to be implemented inside the dash renderer, right?). And even without callbacks, I think this solution is a significant step forward as compared to not being able to pass components at all.

I actually started writing my own very simple render code, which worked in terms of rendering, but it emitted some weird React warnings. After trying to debug it without success, I decided to mimic the behavior of the dash renderer more closely. As a result, the warnings disappeared, but the code is now vastly more complex and contains lot's of code duplicated from the dash renderer. I guess the proper solution here would be to separate out the render functionality in a separate (plotly owned) npm package, which can then be used both by the dash renderer and dash-extensions-js (or the custom component authors directly).

I believe that making the rendering optional for the developer at the prop level is the key to avoid performance issues. We could even provide two options; render a prop with callbacks enabled, or render it without callbacks enabled for improved performance. I believe that would yield the best of both worlds.

How do you think the best possible solution would look in terms of syntax? Would it be that the dash renderer just renders all props with PropTypes.node? Or would it be a library function similar to renderDashComponents that the developer would call? Or something else?

BTW: This discussion is going way beyond the original intent of the PR :)

@snehilvj
Copy link

following

@alexcjohnson
Copy link
Collaborator

Closing this PR: the first part of this (simple props other than children being able to support Dash components) is being worked on in #1965 - please check it out!

The second part, conditionally rendering deeply-nested Dash components, we'll address soon, but not in the next release. @emilhe really appreciate your work here, I expect the eventual built-in solution will closely follow your lead.

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

Successfully merging this pull request may close these issues.

3 participants