Adds support for additionalManifestEntries build option #2124
Conversation
| @@ -104,4 +104,10 @@ module.exports = { | |||
| 'bad-manifest-transforms-return-value': ol`The return value from a | |||
| manifestTransform should be an object with 'manifest' and optionally | |||
| 'warnings' properties.`, | |||
| 'string-entry-warning': ol`Some URLs were passed to additionalManifestEntries | |||
There was a problem hiding this comment.
I'm interested in feedback here: my thinking is that we can support passing in a string URL, but if someone does that, we should let them know about the potential pitfalls involved with precaching a URL that may or may not be revisioned. They can avoid the warning by explicitly using {url: '...'} as the entry, kind of as a "I know what I'm doing and accept the risks mode."
The thing a like about that approach is that the most likely think developers will do first is pass in a string, and they'll see this warning, and hopefully they'll think about whether it's safe to precache that given URL.
An alternative approach would be to disallow passing in a string entirely, but I'm not sure that would be as... educational?... as this approach.
There was a problem hiding this comment.
And another alternative would be to only take in objects with both url and revision properties, and tell developers that they need to explicitly set revision: null if they're sure they're passing in a URL that is already revisioned. That's pretty draconian, though.
There was a problem hiding this comment.
And another alternative would be to only take in objects with both url and revision properties
Yeah, I was going to say exactly this. I think it makes sense to allow strings and show the warning, but the way to silence the warning would be to specify revision: null (or false perhaps).
packages/workbox-build/src/lib/additional-manifest-entries-transform.js
Outdated
Show resolved
Hide resolved
| 'string-entry-warning': ol`Some URLs were passed to additionalManifestEntries | ||
| as strings. Please ensure that these URLs contain versioning info | ||
| (e.g. 'https://example.com/v1.0/index.js'), as Workbox cannot maintain | ||
| its own revision info for them. To disable this warning, pass in an |
There was a problem hiding this comment.
Did you consider making this possible? It wouldn't be terribly hard for use to add our own revision data to an external URL.
There was a problem hiding this comment.
I'm not sure what you mean. Our build tools can't maintain revision info for those URLs, because they're (pretty much by definition) URLs that exist outside of the local build process.
If a developer has their own approach for revisioning these additional URLs then they're free to pass in meaningful info for the revision property instead of null. But in terms of this warning, I'm assuming that if they passed in a string it's because they don't have out-of-band revision info available.
There was a problem hiding this comment.
We could make an HTTP request, hash the contents of the response, and then use that as the revision (just as we do with files on the file system).
Then, if the file changes on the external server, a rebuild with the same user input would invalidate the old URL.
I definitely don't think this should be the default behavior; I'm just thinking it might potentially be a nice feature for developers who don't want/know how to do that themselves.
There was a problem hiding this comment.
I'm afraid we're opening up a can of worms here, because I really don't want developers to use this with unversioned URLs in the way you describe.
If there's a separate deployment process in place for those additional URLs, and it's not tied to generating the service worker, then developers are inevitably going to get confused when the additional URL is updated and their users don't see any updates until they remember to re-run their build process.
There was a problem hiding this comment.
Sure, just wanted to make sure you'd considered it.
There was a problem hiding this comment.
is using webpack's [hash] sufficient enough?
additionalManifestEntries: [
{ url: '/offline.html', revision: '[hash]'}
]There was a problem hiding this comment.
If you have an actual hash value for a given asset derived from the current webpack compilation in a variable, it would be. But the syntax you're using there won't work; workbox-webpack-plugin won't automatically replace '[hash]' inside of additionalManifestEntries for you.
There was a problem hiding this comment.
😅 you're right!
I ended up not needing additionalManifestEntries (for now at least)
| @@ -104,4 +104,10 @@ module.exports = { | |||
| 'bad-manifest-transforms-return-value': ol`The return value from a | |||
| manifestTransform should be an object with 'manifest' and optionally | |||
| 'warnings' properties.`, | |||
| 'string-entry-warning': ol`Some URLs were passed to additionalManifestEntries | |||
There was a problem hiding this comment.
And another alternative would be to only take in objects with both url and revision properties
Yeah, I was going to say exactly this. I think it makes sense to allow strings and show the warning, but the way to silence the warning would be to specify revision: null (or false perhaps).
test/workbox-build/node/lib/additional-manifest-entries-transform.js
Outdated
Show resolved
Hide resolved
PR-Bot Size PluginChanged File Sizes
New FilesNo new files have been added. All File SizesView Table
Workbox Aggregate Size Plugin3.49KB gzip'ed (23% of limit) |
R: @philipwalton
Fixes #1571 by adding in a new
additionalManifestEntriesoption that can be passed in to any of the build tools.