-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Improve resource caching #973
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
Changes from 6 commits
3ba3822
608c0d7
d76804d
be23600
9c3f2fe
daf8d63
aa5e8f7
61fe077
8e6e2f2
5e61384
1933046
c16d069
47d3eb4
6aa42aa
e0847f5
d180533
c71955e
0e091f1
58afb82
f156ede
199c8da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -541,12 +541,15 @@ def _relative_url_path(relative_package_path="", namespace=""): | |
|
||
modified = int(os.stat(module_path).st_mtime) | ||
|
||
return "{}_dash-component-suites/{}/{}?v={}&m={}".format( | ||
return "{}_dash-component-suites/{}/{}.v{}m{}.{}".format( | ||
self.config.requests_pathname_prefix, | ||
namespace, | ||
relative_package_path, | ||
importlib.import_module(namespace).__version__, | ||
'.'.join(relative_package_path.split('.')[:-1]), | ||
importlib.import_module(namespace).__version__.replace( | ||
'.', '_' | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isolated the fingerprinting logic with its inverse in a easily testable file / function |
||
modified, | ||
'.'.join(relative_package_path.split('.')[-1:]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new pattern! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh hmm, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to use a specialized fingerprint building function that should handle these cases consistently in regards to the fingerprint check. https://github.com/plotly/dash/pull/973/files#diff-2b63b39f9669123b2341a57a37913ad8R10 |
||
) | ||
|
||
srcs = [] | ||
|
@@ -676,6 +679,18 @@ def _generate_meta_html(self): | |
|
||
# Serve the JS bundles for each package | ||
def serve_component_suites(self, package_name, path_in_package_dist): | ||
# Check if the resource has a fingerprint | ||
res = re.match( | ||
r'^(.*)[.]v\d+_\d+_\d+(-\w+[.]?\d+)?m\d{10}([.]js)$', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this going to be used for Also wondering if we really need to be that specific with the pattern, and if it really needs to have a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point about the various extensions.
As per #973 (comment) this may indeed be problematic for other file extensions of the form As for the regex being stronger vs. weaker, I was coming at it from the perspective that we have full control over the format that will be provided, so might as well make a strong check here too. That said, there's no obvious advantage to making a stronger check and maybe we end up losing some flexibility if edge-cases appear. Will update with a more general pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK cool, I'll buy that about the maps - if you tell me you've seen it working 😅 Yeah my thinking about the regex was just it's safer to match anything we're sure won't be in the regular filename. Like, what if there's some system where mod time isn't 10 digits for whatever reason, or someone has a really nonstandard version identifier. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yup. They work :) The "routing/fingerprint" done by Dash only affects the resources put in the HTML, in that sense it's more trickery than routing 😸 For the regex, generalized the pattern here https://github.com/plotly/dash/pull/973/files#diff-2b63b39f9669123b2341a57a37913ad8R6 and its inverse. They behave as follow for various valid / invalid strings: These strings are now tested in unit tests within Dash.
My understanding is that this was intentional. Learned about it when I added the |
||
path_in_package_dist, | ||
) | ||
|
||
fingerprint = res is not None | ||
# Resolve real resource name from fingerprinted resource path | ||
path_in_package_dist = ( | ||
res[1] + res[3] if res is not None else path_in_package_dist | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the resource was requested with the correct pattern (from dash filling the html page or from async resources override) - use cache-control, otherwise use Etag |
||
|
||
if package_name not in self.registered_paths: | ||
raise exceptions.DependencyException( | ||
"Error loading dependency.\n" | ||
|
@@ -711,11 +726,28 @@ def serve_component_suites(self, package_name, path_in_package_dist): | |
package.__path__, | ||
) | ||
|
||
return flask.Response( | ||
response = flask.Response( | ||
pkgutil.get_data(package_name, path_in_package_dist), | ||
mimetype=mimetype, | ||
) | ||
|
||
if fingerprint: | ||
Marc-Andre-Rivet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Fingerprinted resources are good forever (1 year) | ||
# No need for ETag as the fingerprint changes with each build | ||
response.cache_control.max_age = 31536000 # 1 year | ||
else: | ||
# Non-fingerprinted resources are given an ETag that | ||
# will be used / check on future requests | ||
response.add_etag() | ||
tag = response.get_etag()[:1][0] | ||
|
||
request_etag = flask.request.headers.get('If-None-Match') | ||
|
||
if '"{}"'.format(tag) == request_etag: | ||
response = flask.Response(None, status=304) | ||
|
||
return response | ||
|
||
def index(self, *args, **kwargs): # pylint: disable=unused-argument | ||
scripts = self._generate_scripts_html() | ||
css = self._generate_css_dist_html() | ||
|
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.
Override webpack's async chunks path resolution; add fingerprint information.