-
-
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
Assets files & index customizations #286
Conversation
dash/dash.py
Outdated
flask.url_for('assets.static', filename=self._favicon)) | ||
else: | ||
favicon = '' | ||
return self.interpolate_index( |
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.
👍
dash/_utils.py
Outdated
def interpolate_str(template, **data): | ||
s = template | ||
for k, v in data.items(): | ||
key = '{' + k + '}' |
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 probably too brittle... in modern Javascript you could easily write something like setConfig({config})
and have someone paste that into index
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.
Think doubling the brackets {{config}}
would be enough or maybe add a character before %{config}
?
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.
doubling is probably not enough... I would do something like {%dash_config%}
or something personally just to make it super clear
dash/dash.py
Outdated
def interpolate_index(self, | ||
metas, title, css, config, | ||
scripts, app_entry, favicon): | ||
return _interpolate(self.index_string, |
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's probably not enough error-checking here... Shouldn't we raise a very helpful and clear message if they forget to add {scripts}
or something?
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.
Two options I thought of:
- check in a property setter on the index_string.
- fast only one check, but no checking on interpolate_index.
- raise when set.
- check after interpolate_index the string contains the ids of required elements.
- check every time the index render.
- raise only when browsing.
Which would be best ?
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 would actually do both :) That way it fails fast for those using index
but it still checks for those using interpolate_index
... better developer experience all around
dash/dash.py
Outdated
def run_server(self, | ||
port=8050, | ||
debug=False, | ||
**flask_run_options): | ||
bp = flask.Blueprint('assets', 'assets', | ||
static_folder=self.config.assets_folder, | ||
static_url_path=self.config.assets_url_path) |
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.
Do we need to ensure a minimum version of flask for this? i.e. >1.0?
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.
It was add in 0.7, we have Flask>=0.12
.
|
||
tags = [] | ||
if not has_charset: | ||
tags.append('<meta charset="UTF-8"/>') |
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 very thoughtful 👍
if len(splitted) > 1: | ||
base = '/'.join(slash_splitter.split(s)) | ||
else: | ||
base = splitted[0] |
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.
Could we use base = os.path.split(walk_dir)[0]
?
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.
NVM, I get it now, we have to replace \
with /
for URL friendly paths.
dash/dash.py
Outdated
{app_entry} | ||
<footer> | ||
{config} | ||
{scripts} |
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.
Could we split out scripts into dash_renderer_scripts
and dash_components_scripts
? The main reason is that dash_renderer
is going to become configurable in when we add in custom JS hooks. So, users will end up needing to do something like:
dash_renderer = DashRenderer({
request_hook: function(...) {
...
}
})
And so it'd be nice if they could replace this:
<!DOCTYPE html>
<html>
<head>
{metas}
<title>{title}</title>
{favicon}
{css}
</head>
<body>
{app_entry}
<footer>
{config}
{dash_renderer}
{dash_component_scripts}
</footer>
</body>
</html>
with something like this:
<!DOCTYPE html>
<html>
<head>
{metas}
<title>{title}</title>
{favicon}
{css}
</head>
<body>
{app_entry}
<footer>
{config}
dash_renderer = DashRenderer({
request_hook: function(...) {
...
}
})
{dash_component_scripts}
</footer>
</body>
</html>
Now, I suppose the other way they could do this would be with the interpolated index function, where they would do something like:
class CustomDash(dash):
def interpolated_index(metas, title, css, config, scripts, _app_entry, favicon):
filtered_scripts = [
script for script in scripts if 'dash_renderer' not in script
]
return '''
<!DOCTYPE html>
<html>
<head>
{metas}
<title>{title}</title>
{favicon}
{css}
</head>
<body>
{app_entry}
<footer>
{config}
dash_renderer = DashRenderer({
request_hook: function(...) {
...
}
})
{filtered_scripts}
</footer>
</body>
</html>
'''.format(metas, title, css, config, filtered_scripts, _app_entry, favicon)
Am I understanding that correctly?
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 wasn't too sure about the js hooks so I left them out of this, right now the scripts are all bundled together in _generate_scripts_html that return a string with all the scripts tags, can't iterate over it but it could change.
I thought we could have a config to disable the includes of dash-renderer. Then the user can append a custom renderer to the scripts resources or include it a custom index.
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 they're customizing the DashRenderer
don't they need to have the tags to load the uncustomized one first?
Overall this is looking really good! A few topics of discussion below. Would love feedback @plotly/dash
I actually like this declarative syntax. This seems like a nice pattern that we could use elsewhere, like as a way to replace
I never really liked the
I'm less fond of this syntax as I prefer if there would only be a single declarative way to do things. Now, I'm biased as I've been in the declarative-or-nothing plotly world for several years, so I'd be happy to hear other perspectives :). Anyway, here are my thoughts:
Would love feedback on this @plotly/dash and community. Are these valid arguments or am I being too pedantic? |
elif 'asset_path' in resource: | ||
static_url = flask.url_for('assets.static', | ||
filename=resource['asset_path']) | ||
srcs.append(static_url) |
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 we should wire in cache-busting URLs while we're here. It's a really common issue in the community forum (e.g. https://community.plot.ly/t/reloading-css-automatically/11065).
I originally thought that we could just do this with a query string with the last modified timestamp but it seems like that's not recommended (https://css-tricks.com/strategies-for-cache-busting-css/#article-header-id-2). Instead, it seems like we'd need to somehow encode it into the resource name. Do you have a sense of how hard that might be?
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.
Although I'm being pretty hypocritical here, looks like I did cache busting with the component libraries with query strings:
Lines 194 to 199 in 3dfa941
return '{}_dash-component-suites/{}/{}?v={}'.format( | |
self.config['routes_pathname_prefix'], | |
namespace, | |
relative_package_path, | |
importlib.import_module(namespace).__version__ | |
) |
🙄
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 did cache busting before with webpack and an extension. Was basically a template render that replaced the hash part of a filename with a new one.
In dash, I think it would be kinda hard to do that, but we could have a watcher on the asset folder, copy those assets with a filename including the hash to a temp static folder, keep the hash in a dict with the path as key and serve the file with the hash formatted in. When a file change, copy it to the temp folder with a new hash and put the new hash as the value for the path. Could also tell the browser to reload while we're at 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.
Yeah that's a good idea. I'm a little worried about introducing a temp folder, I feel like users won't know why it's there or if they should commit it, etc.
Instead of having a watcher, could we just call this function on every page load (while in dev
mode) and get the latest timestamp of the file? And then if we're not in dev
mode, we could store the timestamps on the first page load?
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 temp folder would be created with the tempfile
module and located in the user temp directory (ie %appdata%/local/temp
).
But yea, just checking the timestamps of the files before index and appending that in a query string would be a quick fix.
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.
Cool, sounds like there are a couple of options. So, let's create a new GitHub issue about this and tackle it in a subsequent PR
Could you include an |
0ba032b
to
84acbab
Compare
Let's give members from @plotly/dash another 12 hours to give a second review. If no one else steps in, let's go ahead and merge and release 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 looks great overall. One subtle point I think was missed:
dash/dash.py
Outdated
else: | ||
base = splitted[0] | ||
|
||
for f in files: |
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.
os.walk
calls os.listdir
internally and the os.listdir
docs state "The [returned] list is in arbitrary order" since the user's OS ultimately decides what ordering this returns. Definitely not desirable as apps will behave differently on different machines.
I had a hunch the test case was just getting lucky so I added more files and got this ordering: ['load_first', 'load_after6', 'load_after', 'load_after1', 'load_after2', 'load_after5', 'load_after11', 'load_after4', 'load_after3', 'load_after10', 'load_ after7']
This can be fixed by just adding
for current, _, files in os.walk(walk_dir):
if current == walk_dir:
base = ''
else:
s = current.replace(walk_dir, '').lstrip('\\').lstrip('/')
splitted = slash_splitter.split(s)
if len(splitted) > 1:
base = '/'.join(slash_splitter.split(s))
else:
base = splitted[0]
files.sort() # ADDED LINE: Sort the files!
for f in files:
if base:
path = '/'.join([base, f])
else:
path = f
This sorts the files as: ['load_first', 'load_after', 'load_after1', 'load_after10', 'load_after11', 'load_after2', 'load_after3', 'load_after4', 'load_after5', 'load_after6', 'load_ after7']
I personally would prefer if 'load_after10' and 'load_after11' came last, which can be done by changing
files.sort() -> files.sort(key=lambda f: int('0' + ''.join(filter(str.isdigit, f))))
The former is what is expected, but the latter is what a programmer usually wants when sorting files. Either way I like the way everything else looks and am 💃 once the sorting works.
I can take a look at this shortly, if you hold off merging for a couple of
hours
…On Wed, Jul 25, 2018, 10:47 Ryan Marren ***@***.***> wrote:
***@***.**** commented on this pull request.
This looks great overall. One subtle point I think was missed:
------------------------------
In dash/dash.py
<#286 (comment)>:
> + res['external_url'] = '{}{}'.format(
+ self.config.assets_external_path, path)
+ return res
+
+ for current, _, files in os.walk(walk_dir):
+ if current == walk_dir:
+ base = ''
+ else:
+ s = current.replace(walk_dir, '').lstrip('\\').lstrip('/')
+ splitted = slash_splitter.split(s)
+ if len(splitted) > 1:
+ base = '/'.join(slash_splitter.split(s))
+ else:
+ base = splitted[0]
+
+ for f in files:
os.walk calls os.listdir internally and the os.listdir docs state
<https://docs.python.org/3.6/library/os.html#os.listdir> "The [returned]
list is in arbitrary order" since the user's OS ultimately decides what
ordering this returns. Definitely not desirable as apps will behave
differently on different machines.
I had a hunch the test case was just getting lucky so I added more files
and got this ordering: ['load_first', 'load_after6', 'load_after',
'load_after1', 'load_after2', 'load_after5', 'load_after11', 'load_after4',
'load_after3', 'load_after10', 'load_ after7']
This can be fixed by just adding
for current, _, files in os.walk(walk_dir):
if current == walk_dir:
base = ''
else:
s = current.replace(walk_dir, '').lstrip('\\').lstrip('/')
splitted = slash_splitter.split(s)
if len(splitted) > 1:
base = '/'.join(slash_splitter.split(s))
else:
base = splitted[0]
files.sort() # ADDED LINE: Sort the files!
for f in files:
if base:
path = '/'.join([base, f])
else:
path = f
This sorts the files as: ['load_first', 'load_after', 'load_after1',
'load_after10', 'load_after11', 'load_after2', 'load_after3',
'load_after4', 'load_after5', 'load_after6', 'load_ after7']
I personally would prefer if 'load_after10' and 'load_after11' came last,
which can be done by changing
files.sort() -> files.sort(key=lambda f: int('0' +
''.join(filter(str.isdigit, f))))
The former is what is expected, but the latter is what a programmer
usually wants when sorting files. Either way I like the way everything else
looks and am 💃 once the sorting works.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#286 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACs1GFhwKbKGM8dgElualPuEPeSv3zB8ks5uJ8A4gaJpZM4VLi5g>
.
|
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.
Spotted a couple places for improvements but they're not blockers, so it's a 💃 from me also
(Edit: oops, does the explicit Approve
do anything of significance?)
dash/dash.py
Outdated
kwargs.get('app_entry'), | ||
kwargs.get('config'), | ||
kwargs.get('scripts')) | ||
|
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'd have a preference for using named interpolations here, as I think this is good practice for such a long string being interpolated. eg '{scripts}'
... .format(scripts=kwargs['scripts'])
But it's not that much of an issue.
) | ||
missing = [missing for check, missing in checks if not check] | ||
if missing: | ||
raise Exception( |
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 should be a Dash-specific exception from the dash.exceptions
module, but there's already a couple of other plain Exception
s in dash.py
, so it could just be fixed in another PR.
Chris, is this now functioning? I've made an attempt to replicate your "Simple" and "Override Index" example however, the .css formatting isn't being applied. |
Yeah this is. See https://community.plot.ly/t/dash-version-0-22-0-released/12098 for some official notes. I'm working on some official docs in https://github.com/plotly/dash-docs/pull/126 |
I've been reading through documentation and am having a really hard time understanding how to accomplish what I want. I am trying to include additional tags in the header of the page, not just meta tags. I have both link and script tags I want to include, which doesn't seem to work with the Any thoughts on how to accomplish this? I've also tried to set the index_string but received the following error: |
@jonboone1 by link and script tags do you mean you want to include your own .css and .js files on the page? Those are both included in the page automatically if you put them in the If the resources you want to add are elsewhere on the internet, you can use |
@jonboone1 - recognise this is now 18 months old - but if anyone else stumbles on this with requirements for custom The docs for this are under 'Option 1 - Index string' here. |
Assets includes & index customization
Solution for #265 Proposal for Offline CSS and JS and Customizable
index.html
Assets include
Dash will now look for a folder named
assets
on the current work directory, if one is found, it will walk that directory and include js and css files on the index.Related configs:
Supported files:
.js
, javascript files will be included after the components libs and before the dash-render..css
, stylesheets will be included as a<link>
in the head.favicon.ico
, include as a<link>
in the head.The files are included in alphabetic order.
The directories can be nested.
Advanced use:
To host your assets content externally:
app.scripts.config.serve_locally = False
app.config.assets_external
to your base host url, iehttp://bucket.s3.amazonaws.com/
app.config.include_asset_files
must still be set toTrue
for the files to be indexed by dash.Index customization
Meta tags
It is now possible to add meta tags to the index of dash.
Example:
Customizing the index
Add an
index_string
to change the default index dash use.The
{%key%}
s will be formatted like in the default index.Available keys:
Also added
interpolate_index
method on Dash, override it to get the context values of the index before rendering.I added tests for the meta keys and index customization. For the assets, I made a test in an external project to test if the files are included in that project, @plotly/dash I need help to integrate that test to run with the other tests.