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.

Sign up for GitHub

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

Merged
merged 28 commits into from
Jul 25, 2018
Merged

Assets files & index customizations #286

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
53ec2ef
Add support for static_path resources.
T4rk1n Jul 4, 2018
a14709a
Add static directory walk for files to include.
T4rk1n Jul 4, 2018
69b8adc
pylint fixes.
T4rk1n Jul 4, 2018
7cafb47
Add support for meta tags.
T4rk1n Jul 4, 2018
7ebab39
Add support favicon located in static dir.
T4rk1n Jul 4, 2018
fc26544
Fix static walking nested directories.
T4rk1n Jul 5, 2018
d2cce95
Changed the meta tags dict to a list, added meta_tags to dash.__init__.
T4rk1n Jul 6, 2018
1212ee5
Add test for meta tags.
T4rk1n Jul 6, 2018
846c8fc
Fix bad line that was included in rebase.
T4rk1n Jul 10, 2018
4777731
Add support for static external resources.
T4rk1n Jul 10, 2018
23195f0
Rename `static` to `assets` for user static file includes.
T4rk1n Jul 10, 2018
0c96dc7
Add _generate_meta_html to build html meta tags.
T4rk1n Jul 10, 2018
f943471
Add index customization by string interpolations.
T4rk1n Jul 10, 2018
cd0c15c
Re-add support for favicon in interpolate_index.
T4rk1n Jul 10, 2018
0d9151c
Change add_meta_tag args to a dict to support every meta attributes.
T4rk1n Jul 10, 2018
ff1cff3
Add test for index customization.
T4rk1n Jul 10, 2018
eaf91a1
Add test for assets.
T4rk1n Jul 11, 2018
2197e38
Add checks for index, change the format syntax to {%key%}, more tests.
T4rk1n Jul 11, 2018
ea0d2ca
Change interpolate_index params to kwargs.
T4rk1n Jul 17, 2018
4cf9e5c
Remove `add_meta_tag`.
T4rk1n Jul 17, 2018
f6e9922
Block pylint version to 1.9.2
T4rk1n Jul 18, 2018
8cc781b
Put assets Blueprint in ctor, remove related configs.
T4rk1n Jul 23, 2018
0046402
Use `flask.helpers.get_root_path` for assets folder resolution.
T4rk1n Jul 23, 2018
6af0381
Add docstring to interpolate_index.
T4rk1n Jul 24, 2018
2c104a5
Ensure assets files are ordered, add more test files.
T4rk1n Jul 25, 2018
1171f0a
Update changelog and version.
T4rk1n Jul 25, 2018
cd4cfa9
Merge branch 'master' into assets-index-customizations
T4rk1n Jul 25, 2018
495762e
pylint fixes.
T4rk1n Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions dash/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
def interpolate_str(template, **data):
s = template
for k, v in data.items():
key = '{%' + k + '%}'
s = s.replace(key, v)
return s


class AttributeDict(dict):
"""
Dictionary subclass enabling attribute lookup/assignment of keys/values.
Expand Down
207 changes: 178 additions & 29 deletions dash/dash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
from __future__ import print_function

import os
import sys
import collections
import importlib
import json
import pkgutil
import warnings
import re

from functools import wraps

import plotly
Expand All @@ -19,6 +22,42 @@
from .development.base_component import Component
from . import exceptions
from ._utils import AttributeDict as _AttributeDict
from ._utils import interpolate_str as _interpolate

_default_index = '''
<!DOCTYPE html>
<html>
<head>
{%metas%}
<title>{%title%}</title>
{%favicon%}
{%css%}
</head>
<body>
{%app_entry%}
<footer>
{%config%}
{%scripts%}
</footer>
</body>
</html>
'''

_app_entry = '''
<div id="react-entry-point">
<div class="_dash-loading">
Loading...
</div>
</div>
'''

_re_index_entry = re.compile(r'{%app_entry%}')
_re_index_config = re.compile(r'{%config%}')
_re_index_scripts = re.compile(r'{%scripts%}')

_re_index_entry_id = re.compile(r'id="react-entry-point"')
_re_index_config_id = re.compile(r'id="_dash-config"')
_re_index_scripts_id = re.compile(r'src=".*dash[-_]renderer.*"')


# pylint: disable=too-many-instance-attributes
Expand All @@ -29,8 +68,13 @@ def __init__(
name='__main__',
server=None,
static_folder='static',
assets_folder=None,
assets_url_path='/assets',
include_assets_files=True,
url_base_pathname='/',
compress=True,
meta_tags=None,
index_string=_default_index,
**kwargs):
Copy link
Member

@chriddyp chriddyp Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also create an issue about adding a css_urls and js_script_urls arguments to this init argument as an alternative to our app.css.append_css and app.scripts.append_script:

I actually like this declarative syntax. This seems like a nice pattern that we could use elsewhere, like as a way to replace app.scripts.append_script:

app = dash.Dash(
     js_script_urls=['https://cdn.google.com/google-analytics.js'],
     css_stylesheet_urls=['https://cdn.bootstrap.com/bootstrap.css']
)

I never really liked the app.scripts.append_script syntax and I'd generally prefer for the dash.Dash() object to have as few methods and properties as possible. If everything could just be in the app.config and settable through the dash.Dash() constructor, I'd be very happy.

(from comment #286)


# pylint-disable: too-many-instance-attributes
Expand All @@ -42,20 +86,35 @@ def __init__(
See https://github.com/plotly/dash/issues/141 for details.
''', DeprecationWarning)

name = name or 'dash'
self._assets_folder = assets_folder or os.path.join(
flask.helpers.get_root_path(name), 'assets'
)

# allow users to supply their own flask server
self.server = server or Flask(name, static_folder=static_folder)

self.server.register_blueprint(
flask.Blueprint('assets', 'assets',
static_folder=self._assets_folder,
static_url_path=assets_url_path))

self.url_base_pathname = url_base_pathname
self.config = _AttributeDict({
'suppress_callback_exceptions': False,
'routes_pathname_prefix': url_base_pathname,
'requests_pathname_prefix': url_base_pathname
'requests_pathname_prefix': url_base_pathname,
'include_assets_files': include_assets_files,
'assets_external_path': '',
})

# list of dependencies
self.callback_map = {}

self._index_string = ''
self.index_string = index_string
self._meta_tags = meta_tags or []
self._favicon = None

if compress:
# gzip
Compress(self.server)
Expand Down Expand Up @@ -149,12 +208,26 @@ def layout(self, value):
# pylint: disable=protected-access
self.css._update_layout(layout_value)
self.scripts._update_layout(layout_value)
self._collect_and_register_resources(
self.scripts.get_all_scripts()
)
self._collect_and_register_resources(
self.css.get_all_css()

@property
def index_string(self):
return self._index_string

@index_string.setter
def index_string(self, value):
checks = (
(_re_index_entry.search(value), 'app_entry'),
(_re_index_config.search(value), 'config',),
(_re_index_scripts.search(value), 'scripts'),
)
missing = [missing for check, missing in checks if not check]
if missing:
raise Exception(
Copy link
Contributor

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 Exceptions in dash.py, so it could just be fixed in another PR.

'Did you forget to include {} in your index string ?'.format(
', '.join('{%' + x + '%}' for x in missing)
)
)
self._index_string = value

def serve_layout(self):
layout = self._layout_value()
Expand All @@ -180,6 +253,7 @@ def serve_routes(self):
)

def _collect_and_register_resources(self, resources):
# now needs the app context.
# template in the necessary component suite JS bundles
# add the version number of the package as a query parameter
# for cache busting
Expand Down Expand Up @@ -217,8 +291,12 @@ def _relative_url_path(relative_package_path='', namespace=''):
srcs.append(url)
elif 'absolute_path' in resource:
raise Exception(
'Serving files form absolute_path isn\'t supported yet'
'Serving files from absolute_path isn\'t supported yet'
)
elif 'asset_path' in resource:
static_url = flask.url_for('assets.static',
filename=resource['asset_path'])
srcs.append(static_url)
Copy link
Member

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?

Copy link
Member

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:

dash/dash/dash.py

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__
)

🙄

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

return srcs

def _generate_css_dist_html(self):
Expand Down Expand Up @@ -260,6 +338,20 @@ def _generate_config_html(self):
'</script>'
).format(json.dumps(self._config()))

def _generate_meta_html(self):
has_charset = any('charset' in x for x in self._meta_tags)

tags = []
if not has_charset:
tags.append('<meta charset="UTF-8"/>')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very thoughtful 👍

for meta in self._meta_tags:
attributes = []
for k, v in meta.items():
attributes.append('{}="{}"'.format(k, v))
tags.append('<meta {} />'.format(' '.join(attributes)))

return '\n '.join(tags)

# Serve the JS bundles for each package
def serve_component_suites(self, package_name, path_in_package_dist):
if package_name not in self.registered_paths:
Expand Down Expand Up @@ -294,28 +386,47 @@ def index(self, *args, **kwargs): # pylint: disable=unused-argument
scripts = self._generate_scripts_html()
css = self._generate_css_dist_html()
config = self._generate_config_html()
metas = self._generate_meta_html()
title = getattr(self, 'title', 'Dash')
return '''
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>{}</title>
{}
</head>
<body>
<div id="react-entry-point">
<div class="_dash-loading">
Loading...
</div>
</div>
<footer>
{}
{}
</footer>
</body>
</html>
'''.format(title, css, config, scripts)
if self._favicon:
favicon = '<link rel="icon" type="image/x-icon" href="{}">'.format(
flask.url_for('assets.static', filename=self._favicon))
else:
favicon = ''

index = self.interpolate_index(
metas=metas, title=title, css=css, config=config,
scripts=scripts, app_entry=_app_entry, favicon=favicon)

checks = (
(_re_index_entry_id.search(index), '#react-entry-point'),
(_re_index_config_id.search(index), '#_dash-configs'),
(_re_index_scripts_id.search(index), 'dash-renderer'),
)
missing = [missing for check, missing in checks if not check]

if missing:
plural = 's' if len(missing) > 1 else ''
raise Exception(
'Missing element{pl} {ids} in index.'.format(
ids=', '.join(missing),
pl=plural
)
)

return index

def interpolate_index(self,
metas='', title='', css='', config='',
scripts='', app_entry='', favicon=''):
Copy link
Member

@chriddyp chriddyp Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include a quick docstring for our users. Something like:

A function that is called to create the initial HTML string that is loaded on the page.
Override this function to provide your own HTML string. For example:

class DashOverride:
    def interpolate_index(...)
         ...

(but with the example filled out)

return _interpolate(self.index_string,
Copy link
Contributor

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?

Copy link
Contributor Author

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 ?

Copy link
Contributor

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

metas=metas,
title=title,
css=css,
config=config,
scripts=scripts,
favicon=favicon,
app_entry=app_entry)

def dependencies(self):
return flask.jsonify([
Expand Down Expand Up @@ -558,9 +669,47 @@ def dispatch(self):
return self.callback_map[target_id]['callback'](*args)

def _setup_server(self):
if self.config.include_assets_files:
self._walk_assets_directory()

self._generate_scripts_html()
self._generate_css_dist_html()

def _walk_assets_directory(self):
walk_dir = self._assets_folder
slash_splitter = re.compile(r'[\\/]+')

def add_resource(p):
res = {'asset_path': p}
if self.config.assets_external_path:
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]
Copy link
Member

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]?

Copy link
Member

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.


for f in files:
Copy link
Contributor

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.

if base:
path = '/'.join([base, f])
else:
path = f

if f.endswith('js'):
self.scripts.append_script(add_resource(path))
elif f.endswith('css'):
self.css.append_css(add_resource(path))
elif f == 'favicon.ico':
self._favicon = path

def run_server(self,
port=8050,
debug=False,
Expand Down
6 changes: 3 additions & 3 deletions dash/resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ def _filter_resources(self, all_resources):
filtered_resource = {}
if 'namespace' in s:
filtered_resource['namespace'] = s['namespace']

if 'external_url' in s and not self.config.serve_locally:
filtered_resource['external_url'] = s['external_url']
elif 'relative_package_path' in s:
Expand All @@ -30,6 +29,8 @@ def _filter_resources(self, all_resources):
)
elif 'absolute_path' in s:
filtered_resource['absolute_path'] = s['absolute_path']
elif 'asset_path' in s:
filtered_resource['asset_path'] = s['asset_path']
elif self.config.serve_locally:
warnings.warn(
'A local version of {} is not available'.format(
Expand Down Expand Up @@ -112,8 +113,7 @@ class config:
serve_locally = False


class Scripts:
# pylint: disable=old-style-class
class Scripts: # pylint: disable=old-style-class
def __init__(self, layout=None):
self._resources = Resources('_js_dist', layout)
self._resources.config = self.config
Expand Down
2 changes: 1 addition & 1 deletion dev-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ six
plotly>=2.0.8
requests[security]
flake8
pylint
pylint==1.9.2
1 change: 1 addition & 0 deletions tests/assets/load_first.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
window.tested = ['load_first'];
3 changes: 3 additions & 0 deletions tests/assets/nested_css/nested.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#content {
padding: 8px;
}
1 change: 1 addition & 0 deletions tests/assets/nested_js/load_after.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
window.tested.push('load_after');
2 changes: 2 additions & 0 deletions tests/assets/nested_js/load_after1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
window.tested.push('load_after1');
document.getElementById('tested').innerHTML = JSON.stringify(window.tested);
1 change: 1 addition & 0 deletions tests/assets/reset.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
body {margin: 0;}
Loading