Skip to content

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.

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

Merged
merged 21 commits into from
Oct 28, 2019
Merged

Improve resource caching #973

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
3ba3822
add use of etag / if-none-match
Oct 22, 2019
608c0d7
black
Oct 22, 2019
d76804d
handling tuples
Oct 23, 2019
be23600
etag or fingerprint
Oct 23, 2019
9c3f2fe
lint
Oct 23, 2019
daf8d63
update resource tests
Oct 23, 2019
aa5e8f7
py2.7
Oct 23, 2019
61fe077
lint
Oct 23, 2019
8e6e2f2
clean up
Oct 23, 2019
5e61384
clean up
Oct 23, 2019
1933046
changelog
Oct 23, 2019
c16d069
- improve fingerprinting
Oct 24, 2019
47d3eb4
update handling of `@` in fingerprinting and test (for [email protected]…
Oct 24, 2019
6aa42aa
add tests for version and hash
Oct 24, 2019
e0847f5
clean up
Oct 24, 2019
d180533
hash
Oct 24, 2019
c71955e
fix error adding fingerprint to url
Oct 24, 2019
0e091f1
Update dash/dash.py
Marc-Andre-Rivet Oct 28, 2019
58afb82
Merge branch 'dev' into early-renderer
Marc-Andre-Rivet Oct 28, 2019
f156ede
plugin - distinguish local from remote script resources
Oct 28, 2019
199c8da
Merge branch 'early-renderer' of github.com:plotly/dash into early-re…
Oct 28, 2019
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
2 changes: 1 addition & 1 deletion @plotly/webpack-dash-dynamic-import/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@plotly/webpack-dash-dynamic-import",
"version": "1.0.0",
"version": "1.1.0",
"description": "Webpack Plugin for Dynamic Import in Dash",
"repository": {
"type": "git",
Expand Down
27 changes: 24 additions & 3 deletions @plotly/webpack-dash-dynamic-import/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,16 @@
const resolveImportSource = `\
const fs = require('fs');

function getFingerprint() {
const package = fs.readFileSync('./package.json');
const packageJson = JSON.parse(package);

const timestamp = Math.round(Date.now() / 1000);
const version = packageJson.version.replace(/[.]/g, '_');

return `"v${version}m${timestamp}"`;
}

const resolveImportSource = () => `\
Object.defineProperty(__webpack_require__, 'p', {
get: (function () {
let script = document.currentScript;
Expand All @@ -15,15 +27,24 @@ Object.defineProperty(__webpack_require__, 'p', {
return url;
};
})()
});`
});

const __jsonpScriptSrc__ = jsonpScriptSrc;
jsonpScriptSrc = function(chunkId) {
const srcFragments = __jsonpScriptSrc__(chunkId).split('.');
srcFragments.splice(-1, 0, ${getFingerprint()});

return srcFragments.join('.');
}
Copy link
Contributor Author

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.

`

class WebpackDashDynamicImport {
apply(compiler) {
compiler.hooks.compilation.tap('WebpackDashDynamicImport', compilation => {
compilation.mainTemplate.hooks.requireExtensions.tap('WebpackDashDynamicImport > RequireExtensions', (source, chunk, hash) => {
return [
source,
resolveImportSource
resolveImportSource()
]
});
});
Expand Down
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).
## Unreleased
### Added
- [#964](https://github.com/plotly/dash/pull/964) Adds support for preventing
updates in clientside functions.
updates in clientside functions.
- Reject all updates with `throw window.dash_clientside.PreventUpdate;`
- Reject a single output by returning `window.dash_clientside.no_update`
- [#973](https://github.com/plotly/dash/pull/973) Adds support for resource caching and adds a fallback caching mechanism through etag

## [1.4.1] - 2019-10-17
### Fixed
Expand Down
41 changes: 37 additions & 4 deletions dash/dash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
'.', '_'
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new pattern!

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh hmm, .js.map again...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 = []
Expand Down Expand Up @@ -676,6 +679,19 @@ 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)$',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this going to be used for .css and .js.map too? Anyway those are included in the mimetype dict below.

Also wondering if we really need to be that specific with the pattern, and if it really needs to have a . in the (-\w+[.]?\d+) group? What about:

r'^(.*)[.]v[\w_-]+m\d+([.][\w.]+)$'

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Good point about the various extensions.

.js.map is not really an issue here as the requested resource comes from the //# sourceMappingURL=bundle.js.map at the end of the bundle file, is handled by the browser and will never get fingerprinted in the front-end or be provided by the sever in the html. It will get an etag but I'm not really sure if that means anything for a source-map.

As per #973 (comment) this may indeed be problematic for other file extensions of the form xx.yy. Instead of operating under the assumption that only the last group is the extension, could be updated to only use the first group as the filename.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 mimetype dict as written right now is explicit - whether or not intentionally so - that the final extension can only be js, css, or map - anything else will throw a KeyError. Currently anything else you need would go through assets (which would be interesting to investigate for caching, though generally much less important than component js), not component suites.

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

if you tell me you've seen it working 😅

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:

image

image

These strings are now tested in unit tests within Dash.

The mimetype dict as written right now is explicit

My understanding is that this was intentional. Learned about it when I added the map extension. There's apparently a library that offers mimetype mapping but I was told at the time that it was sketchy. Needs to be revisited. If only for the woff/ddk issue.

path_in_package_dist,
)

# Resolve real resource name from fingerprinted resource path
path_in_package_dist = (
res.group(1) + res.group(3)
if res is not None
else path_in_package_dist
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"
Expand Down Expand Up @@ -711,11 +727,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 res is not None:
# 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()[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()
Expand Down
10 changes: 5 additions & 5 deletions tests/unit/test_resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def test_external(mocker):
mocker.patch("dash_core_components._js_dist")
mocker.patch("dash_html_components._js_dist")
dcc._js_dist = _monkey_patched_js_dist # noqa: W0212
dcc.__version__ = 1
dcc.__version__ = "1.0.0"

app = dash.Dash(
__name__, assets_folder="tests/assets", assets_ignore="load_after.+.js"
Expand Down Expand Up @@ -66,7 +66,7 @@ def test_internal(mocker):
mocker.patch("dash_core_components._js_dist")
mocker.patch("dash_html_components._js_dist")
dcc._js_dist = _monkey_patched_js_dist # noqa: W0212,
dcc.__version__ = 1
dcc.__version__ = "1.0.0"

app = dash.Dash(
__name__, assets_folder="tests/assets", assets_ignore="load_after.+.js"
Expand All @@ -83,10 +83,10 @@ def test_internal(mocker):

assert resource == [
"/_dash-component-suites/"
"dash_core_components/external_javascript.js?v=1&m=1",
"dash_core_components/external_javascript.v1_0_0m1.js",
"/_dash-component-suites/"
"dash_core_components/external_css.css?v=1&m=1",
"/_dash-component-suites/" "dash_core_components/fake_dcc.js?v=1&m=1",
"dash_core_components/external_css.v1_0_0m1.css",
"/_dash-component-suites/" "dash_core_components/fake_dcc.v1_0_0m1.js",
]

assert (
Expand Down