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

Children should never be undefined in JSON passed to dash-renderer; results in reducer "layout" error #27

Closed
rpkyle opened this issue May 20, 2020 · 5 comments · Fixed by #28
Assignees
Labels
bug Something isn't working

Comments

@rpkyle
Copy link
Contributor

rpkyle commented May 20, 2020

A simple test app with two html_div components declared and one html_br in between them fails to render anything in the browser window:

using Dash
using DashHtmlComponents

app = dash()
app.layout = html_div(id="outer-div") do
    html_div("A div", id="first-inner-div"),
    html_br(),
    html_div("Another div", id="second-inner-div")
end

run_server(app)

In the JavaScript console, the following error appears:
image

However, if you pass a blank space -- e.g. " " into the children property, it works. The problem may appear for components with all default arguments passed, that are nested inside another component, though this hypothesis has not been extensively tested.

The behaviour is reproducible if html_br() is substituted with html_div() or html_p().

@waralex @Marc-Andre-Rivet

@rpkyle rpkyle added the bug Something isn't working label May 20, 2020
@rpkyle rpkyle changed the title html_br component does not appear to work properly Children with nested components using all default properties do not appear to work properly May 20, 2020
@rpkyle rpkyle changed the title Children with nested components using all default properties do not appear to work properly Reducer "layout" returned undefined error when nesting components with all default properties May 20, 2020
@rpkyle
Copy link
Contributor Author

rpkyle commented May 20, 2020

Additional context, the results of returning _dash-layout for a Julia app (left pane) and the comparable Python app (right pane):

image

@waralex It looks like we're not passing a default value for children when it's not provided (should be the Julia equivalent of Python's None or R's NULL), and this makes dash-renderer unhappy, since children cannot be undefined.

Comparing the Python, R, and Julia implementations of this component, the problem appears to be here:

function html_br(; kwargs...)
        available_props = Set(Symbol[:children, :id, :n_clicks, :n_clicks_timestamp, :key, :role, :accessKey, :className, :contentEditable, :contextMenu, :dir, :draggable, :hidden, :lang, :spellCheck, :style, :tabIndex, :title, :loading_state])
        wild_props = Set(Symbol[Symbol("data-"), Symbol("aria-")])
        wild_regs = r"^(?<prop>data-|aria-)"

        result = Component("Br", "dash_html_components", Dict{Symbol, Any}(), available_props, Set(Symbol[Symbol("data-"), Symbol("aria-")]))

If I'm reading this correctly, available_props should all probably take default values of nothing, I believe the renderer is seeing these passed as "undefined" rather than null (within JSON).

@rpkyle rpkyle changed the title Reducer "layout" returned undefined error when nesting components with all default properties Children should never be undefined in JSON passed to dash-renderer; results in reducer "layout" error May 20, 2020
@alexcjohnson
Copy link
Contributor

I'd also be happy to consider this a renderer bug - children isn't a required prop, in fact if I look at components that don't have a children prop at all (like dcc.Location) their layout entries don't have a children so (a) requiring it to always be provided seems like a waste of bytes and (b) how come it's an error for <br> when it's not an error for components that don't have children?

@rpkyle
Copy link
Contributor Author

rpkyle commented May 20, 2020

I'd also be happy to consider this a renderer bug - children isn't a required prop, in fact if I look at components that don't have a children prop at all (like dcc.Location) their layout entries don't have a children so (a) requiring it to always be provided seems like a waste of bytes and (b) how come it's an error for <br> when it's not an error for components that don't have children?

@alexcjohnson I think that's the general consensus between the three of us (@waralex, @Marc-Andre-Rivet and me) -- at best this renderer behaviour is idiosyncratic, and at worst it's a bug.

But realistically there is still functionality to permit within Dash.jl, since htmlBr()/html.Br() work in R and Python without arguments being passed. This is not super urgent, but I'd argue that it be considered blocking for the release, so however we fix it, we should try to do it in the next week or two, I think.

@waralex waralex mentioned this issue May 20, 2020
@waralex waralex linked a pull request May 20, 2020 that will close this issue
@alexcjohnson
Copy link
Contributor

Copying over my comment from Slack, just to have it all in one place:

I think there’s something deeper going on here, but I don’t understand it. If I load the app @rpkyle posted at the head, then I go look at the request in the network tab, it has no content at all - that’s where the “reducer layout returned undefined” ultimately comes from. And yet if I make that request separately (http://localhost:8050/_dash-layout) I see the JSON with props: {}

But if I modify the Python side to include props: {} everything works fine

@rpkyle said:

interesting, yeah, the R app also has props: {} which was somewhat surprising

Me:

yeah that empty props is somehow triggering the empty response, but it’s not the root cause so fixing this is not the answer, it’ll just come back somewhere else.

So it seems this is not a renderer bug, though where it's really coming from is still unclear.

waralex added a commit that referenced this issue May 20, 2020
@rpkyle
Copy link
Contributor Author

rpkyle commented May 20, 2020

Fixed by #28.

@rpkyle rpkyle closed this as completed May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants