Fix #634: Simplify service worker caching #682
Conversation
README.md
Outdated
| # Extension Loading | ||
|
|
||
| Bramble loads a set of extensions defined in `src/extensions/bramble-extensions.json`. You can | ||
| add remove from this list. You can also temporarily disable extensions by using `?disableExtensions`. |
There was a problem hiding this comment.
I'm not sure what you intended to say here but it currently reads "You can add remove from this list."
README.md
Outdated
|
|
||
| Bramble loads a set of extensions defined in `src/extensions/bramble-extensions.json`. You can | ||
| add remove from this list. You can also temporarily disable extensions by using `?disableExtensions`. | ||
| For example, to disable QuickView and CSSCodeHints load Bramble with `?disableExtensions=QuickView,CSSCodeHints` on the URL. |
There was a problem hiding this comment.
nit: "For example: to [...], load Bramble [...] URL."
src/extensions/README.md
Outdated
| Here `path` refers to the path under `src/` where the extension's dir lives. | ||
| The optional `copy` array includes file path globs to be used when copying | ||
| files from `src/` to `dist/` for this extension at build time. Many extensions | ||
| have no external dependencies, other than the `main.js` file and any modules it |
There was a problem hiding this comment.
nit: looking at bramble-extensions.json file only four out fifteen don't have external dependencies, so it might be better to phrase this as the exception rather than the baseline.
src/utils/BrambleExtensionLoader.js
Outdated
| var brambleExtensions = extensionInfo.map(function(info) { | ||
| return info.path; | ||
| }); | ||
|
|
||
| if(disableExtensions) { | ||
| disableExtensions.split(",").forEach(function (ext) { |
There was a problem hiding this comment.
would this be better as a filter?
if (disableExtensions) {
let disabledExtensions = disableExtensions.split(","); // might even be able to safely reassign to disableExtensions
brambleExtensions = brambleExtensions.filter(function(ext) {
return disabledExtensions.indexOf(ext) === -1;
});
}
|
This is ready for review. I've dealt with the |
|
This needs to get rebased and landed after #637, which should go in first. |
There was a problem hiding this comment.
This works pretty well! One thing I noticed that is not directly related to this patch is that we don't gzip the bramble-sw.js file which we probably should and the fix is simple: just to put the swPrecache grunt task above the compress task in grunt-build-browser-compressed
|
Looks like this still needs to be rebased because it has one extra commit - "Fix review issues, rework ?disableExtensions logic" |
|
OK, I fixed the compress stuff too. I actually had to compress first, then do the sw cache, and then compress the service worker file; otherwise it caches uncompressed files, which is wrong. Thanks for the review. |
* Fix mozilla#634: simplify service worker caching * Rework staticFileGlobs to ignore dist/nls, add runtimeCaching for nls, fonts * Fix review issues, rework ?disableExtensions logic * Compress dist/ and bramble-sw.js, call uglify after build-extensions
* Fix mozilla#634: simplify service worker caching * Rework staticFileGlobs to ignore dist/nls, add runtimeCaching for nls, fonts * Fix review issues, rework ?disableExtensions logic * Compress dist/ and bramble-sw.js, call uglify after build-extensions
* Fix mozilla#634: simplify service worker caching * Rework staticFileGlobs to ignore dist/nls, add runtimeCaching for nls, fonts * Fix review issues, rework ?disableExtensions logic * Compress dist/ and bramble-sw.js, call uglify after build-extensions
This depends on (and currently includes) #637, so that should land first.
I'm switching how we do our service worker cache to not depend on manual intervention to add new files, or remove old ones. What we have now is way to easy to mess up, since I'm pretty much the only person who understands it.
I need to figure out the right way to deal with
dist/nls/**/*since this new approach is including all strings for all languages in what we cache, which takes the cache up to 5.31 MB for 192 resources from 3.54 MB for 98 resources (I totally misseddist/nlsbefore, so it's amazing it runs at all!)