Use cache-busted URLs for image assets #4128
Conversation
| @@ -147,3 +151,5 @@ def includeme(config): | |||
| # integration can be configured in app.py | |||
| config.registry['assets_env'] = assets_env | |||
| config.registry['assets_client_env'] = assets_client_env | |||
|
|
|||
| config.add_request_method(assets_env.asset_url, 'asset_url') | |||
There was a problem hiding this comment.
These asset urls are used only in the templates so perhaps a neater way of exposing this function there would be to follow the pattern in app.py and add the asset_url function as a Jinja2 global.
| return [asset_url(path) for path in bundles[bundle]] | ||
| return [self.asset_url(path) for path in bundles[bundle]] | ||
|
|
||
| def asset_url(self, path): |
There was a problem hiding this comment.
This class is h.assets.Environment, so asset_url is possibly a bit redundant. The method above is called urls, so maybe call this one url?
| """ | ||
| Return the cache-busted URL for an asset with a given path. | ||
| """ | ||
| manifest = self.manifest.load() |
There was a problem hiding this comment.
I think we want to be a little careful here, as previously we were doing one call to load() per bundle, and now we're doing one call to load per file (including when those files are referenced from a bundle).
The manifest is cached, sure, but this is still going to do at least one stat() call, which isn't ideal if we're doing it dozens of times during a single request. Especially in production, where we know the manifest isn't going to change, it might be nice to load it just once.
| @@ -208,7 +208,7 @@ gulp.task('watch-images', function () { | |||
| gulp.watch(imageFiles, ['build-images']); | |||
| }); | |||
|
|
|||
| var MANIFEST_SOURCE_FILES = 'build/@(fonts|images|scripts|styles)/*.@(js|css|woff|jpg|png|svg)'; | |||
There was a problem hiding this comment.
This seems to already cover the relevant file extensions, so do we need to change it?
There was a problem hiding this comment.
It doesn't cover sourcemaps (.{js,css}.map) or subdirectories (images/icons/foo.svg)
Replace hard-coded '/asset' URL paths in templates with cache-busted URLs generated from the site's static asset manifest. * Add `asset_url` helper for retrieving the cache-busted URL for an asset with a given path and use it in templates. * Fix missing entries in asset manifest by including subdirectories (eg. 'images/icons/*.svg') and all extensions (eg. sourcemaps) See #4117
robertknight
force-pushed
the
cache-bust-image-urls
branch
from
91d3ec7 to
a98a39e
Compare
Current coverage is 83.54% (diff: 80.00%)@@ master hypothesis/h#4128 diff @@
==========================================
Files 158 158
Lines 6167 6192 +25
Methods 0 0
Messages 0 0
Branches 698 704 +6
==========================================
+ Hits 5144 5173 +29
+ Misses 948 944 -4
Partials 75 75
|
|
Ok, changes LGTM. I'd like you to review the performance of the application when this goes out and ensure that all the extra |
Replace hard-coded '/asset' URL paths in templates with cache-busted
URLs generated from the site's static asset manifest.
Add
asset_urlhelper for retrieving the cache-busted URL for anasset with a given path and use it in templates.
Fix missing entries in asset manifest by including subdirectories
(eg. 'images/icons/*.svg') and all extensions (eg. sourcemaps)
See hypothesis/h-assets#21