-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
1193 multipage validation #1201
Conversation
looks like dropdown is generating warnings from React ATM
@@ -730,11 +730,7 @@ def multipage_app(validation=False): | |||
layout_page_2 = html.Div( | |||
[ | |||
html.H2("Page 2"), | |||
dcc.Dropdown( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dcc.Dropdown
is giving React deprecated lifecycle method warnings ATM... I thought we had already updated all of these in the 16.13 push? Anyway switched to Input
to get my assert not dash_duo.get_logs()
clean console test to pass.
dash/dash.py
Outdated
_validate.validate_layout(value, layout_value) | ||
self.validation_layout = simple_clone( | ||
# pylint: disable=protected-access | ||
layout_value, [simple_clone(c) for c in layout_value._traverse_ids()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this validation layout as simple as possible, to minimize its overhead on the page:
- strip out components with no ID
- strip out most prop values, at least in Py3 where we can fully inspect the component constructor signature so we know what props are required.
This step is only run once when you set the layout, and even then only when you set it to a function AND you haven't already set app.validation_layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're just using this to validate IDs w.r.t. callbacks, why do we need the required values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also validate props against the component type, so we can't just create dummy components, they have to be the same type as the real components. And unless I were to recreate the JSON serialization for components, the easiest way to do that is to actually instantiate the same component class. But doing so requires that we include required props, because the generated components enforce the required props during construction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhhhh by "props against the component type" do you mean during callback validation with messages like e.g. "You registered a callback to update a Div's childrenz property. However, Div does not have a "children" property."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. "You registered a callback to update a Div's childrenz property. However, Div does not have a "childrenz" property."?
exactly.
const validateIds = !config.suppress_callback_exceptions; | ||
let layout, paths; | ||
if (validateIds && config.validation_layout) { | ||
layout = config.validation_layout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because validation_layout
is pruned down, so faster to crawl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just saying if we have a separate validation layout, that's the one we need to check the callbacks against. The fact that it's pruned should make computePaths
(next line) faster, but the main reason for pruning was to reduce the size of _dash-config
on the index page - I was imagining it could become enormous, if you have multiple tables and graphs on different pages all concatenated together.
dash/dash.py
Outdated
self._layout = value | ||
|
||
# for using flask.has_request_context() to deliver a full layout for | ||
# validation inside a layout function - track if a user might be doing this. | ||
if self._layout_is_function and not self.validation_layout: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is something we should turn off if debug=False
too, or introduce a new dev_tools_validate_layout
function. Just to reduce the performance hit if there are many small components (e.g. a dash_html_components.Table
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Those mostly wouldn't have IDs, so wouldn't be sent to the front end anyway, but with debug off we don't need it at all. In fact I guess we should be able to short-circuit all the front-end validation in that case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added clauses to skip this when suppress_callback_validation
is used, but didn't remove it entirely when debug=False
. There's more to think about here - on the one hand, without this validation the app behavior can change, and on the other there's a lot more validation we could be skipping if that's what we want to do. I've created #1206 to follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👯 once debug is considered
I confirm the fix works on my app (tested with |
Fixes #1193 - the
flask.has_request_context
trick inside anapp.layout
function can be used again to supply a special layout just for validation purposes.In addition, added a new attribute
app.validation_layout
that does the same thing more explicitly.Contributor Checklist
CHANGELOG.md