-
-
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
Add hooks #3029
Add hooks #3029
Conversation
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.
some questions - happy to chat
dash/_hooks.py
Outdated
|
||
def layout(func): | ||
""" | ||
Run a function when serving the layout, the return value |
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 looks like it saves the function func
but doesn't run it - is the comment incorrect?
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.
No, it is only run when
serving the layout, the docstring are placeholder, we can improve the wording.
dash/_hooks.py
Outdated
|
||
def wrap(func): | ||
_name = name or func.__name__ | ||
_ns["routes"].append((_name, func, methods)) |
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.
hm - so some of the _ns
entries are functions and some are tuples, so the structure of entries in _ns["key"]
depends on the value of "key"
?
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.
The _ns
just saves the info for the different hooks, it isn't meant for public usage. For routes/callbacks it needs more arguments so it's saved in a tuple for easy unpacking.
dash/_hooks.py
Outdated
return wrap | ||
|
||
|
||
def error(func: _t.Callable[[Exception], _t.Any]): |
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.
you're declaring the type of func
here but you don't declare types in the previous registration functions - declare them all for consistency?
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.
There is some issue with cyclic dependencies with the other types, I'll see what I can do.
dash/_hooks.py
Outdated
""" | ||
|
||
def wrap(func): | ||
_ns["callback"].append((list(args), dict(kwargs), func)) |
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.
again, it looks like the structure of values stored in _ns
depends on the key, which feels like it should be documented somewhere in this file to help the next person reading the code.
|
||
|
||
class HooksManager: | ||
_registered = False |
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.
please add a comment explaining what _registered
is for - in particular, why is it a class-level variable instead of an instance variable?
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.
see above
dash/_hooks.py
Outdated
|
||
for dist in importlib.metadata.distributions(): | ||
for entry in dist.entry_points: | ||
if entry.group != "dash": |
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.
hm - hard-coded string embedded in the file - define as a constant at the top of the file to make it easier to find? and a comment here explaining what this filtering is doing would be welcome - I don't really understand it.
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 just a filter on the entry_points
of a setup.py
, the entry_points are used to add cli and other functionalities to python packages. I usually don't define a variable if it's only used one time.
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.
a few questions for my own curiosity but I think this is ready to merge
@@ -0,0 +1,222 @@ | |||
import typing as _t |
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.
just curious: why this rather than importing the specific things you need from typing
(e.g., from typing import TypeVar
)?
|
||
|
||
# pylint: disable=too-few-public-methods | ||
class _Hook(_tx.Generic[HookDataType]): |
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.
docstrings? or do we not document internal classes like this?
"callback": [], | ||
"index": [], | ||
} | ||
self._js_dist = [] |
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.
what is _dist
short for?
dash/_hooks.py
Outdated
self._finals = {} | ||
|
||
def add_hook( | ||
self, hook: str, func: _t.Callable, priority=None, final=False, data=None |
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.
no types for priority
, final
, and data
?
self._ns[hook] = sorted(hks, reverse=True, key=lambda h: h.priority) | ||
|
||
def get_hooks(self, hook: str) -> _t.List[_Hook]: | ||
final = self._finals.get(hook, None) |
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 just to ensure that final
is the last hook in this list? if so, maybe a comment to that effect?
dash/_hooks.py
Outdated
"""Add stylesheets to the page.""" | ||
self._css_dist.extend(distribution) | ||
|
||
def index(self, priority=None, final=False): |
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 don't know what "the index" means in this context?
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.
The index is the raw html of the served dash index (like index.html served for '/').
|
||
|
||
class HooksManager: | ||
_registered = False |
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.
see above
args, | ||
kwargs, | ||
) in self._hooks.hooks._clientside_callbacks: | ||
_callback.register_clientside_callback( |
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.
nice and clean
"ResourceType", | ||
{ | ||
"namespace": str, | ||
"async": _t.Union[bool, _t.Literal["eager", "lazy"]], |
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.
so legal values are True
, False
, "eager"
, and "lazy"
? Are these documented somewhere?
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.
Yes those are the valid values, along with None/undefined for all of them properties since total=False
set them as optional.
This is mostly for usage of async
chunks of components libraries (dcc.Graph, dcc.Markdown, dash_table.DataTable). The usual value is True
or undefined, eager or lazy are abstractions that are not very well documented, it is commented here:
Lines 66 to 72 in d26c9d3
# Async assigns a value dynamically to 'dynamic' | |
# based on the value of 'async' and config.eager_loading | |
# | |
# True -> dynamic if the server is not eager, False otherwise | |
# 'lazy' -> always dynamic | |
# 'eager' -> dynamic if server is not eager | |
# (to prevent ever loading it) |
With Dash(eager_loading=True)
, the async: "eager"
chunks normally loaded when the component mounts, would be loaded on page load.
@@ -0,0 +1,188 @@ | |||
from flask import jsonify |
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.
nice
Add
dash.hooks
system to enhance created apps with functionality. These hooks can be automatically added to apps without imports by adding adash
entry toentry_points
insetup.py
.Hooks:
layout
: wraps the layout before serving. Take the layout as first argument and the return value is used as the new layout.setup
: Called when aDash.__init__
is called, used to get a reference to the app.callback
: Add a callback to all the apps. Same signature asdash.callback
.route
: Add a route to the flask server, the route will be prefixed withroutes_pathname_prefix
.error
: Add a global error handler to be used with callbacks.