Support for importWorkboxFrom: <chunkname> and associated fixes #1141
Conversation
There was a problem hiding this comment.
I've added a few minor comments - largely minor refactors.
Is there any chance you could write up some of the higher level approaches taken in the webpack plugin on the Wiki.
Specifically, I don't know the different between a chunk and asset or who they are introduced into a webpack build process. A wiki page with just little info would be super helpful.
| } | ||
|
|
||
| default: { | ||
| for (const chunk of compilation.chunks) { |
There was a problem hiding this comment.
can you add a comment here on what the intended behavior is. My initial thoughts from the bug was that importWorkboxFrom had to be a specific value but it looks like importWorkboxFrom: 'foo' is valid, but I'm not sure what happens.
| for (const chunk of compilation.chunks) { | ||
| if (chunk.name === importWorkboxFrom) { | ||
| workboxSWImport = chunk.files; | ||
| this.config.excludeChunks = (this.config.excludeChunks || []) |
There was a problem hiding this comment.
could you move this initialisation out of this switch statement just to seperate the initialisation and the addition of the chunk name.
| const manifestFilename = `precache-manifest.${fileManifestHash}.js`; | ||
| compilation.assets[manifestFilename] = fileManifestAsset; | ||
|
|
||
| this.config.importScripts = (this.config.importScripts || []) |
There was a problem hiding this comment.
Could the initialisation be moved as well.
| @@ -31,8 +31,7 @@ function sanitizeConfig(config) { | |||
| const propertiesToRemove = [ | |||
| 'chunks', | |||
| 'excludeChunks', | |||
| 'filename', | |||
| 'manifestFilename', | |||
There was a problem hiding this comment.
Are these completely removed as options or is it just moved logic wise to somewhere else?
There was a problem hiding this comment.
They're removed (and hardcoded for the time being; some of this will be revised when we support GenerateSW vs. InjectManifest).
|
Thanks; I've written up the state of the webpack stuff at https://github.com/GoogleChrome/workbox/wiki/webpack%2C-assets%2C-and-chunks-%28oh-my%21%29, which was a good exercise anyway. |
PR-Bot Size PluginChanged File Sizes
New FilesNo new files have been added. All File SizesView Table
Workbox Aggregate Size Plugin☠️ WARNING ☠️We are using 152% of our max size budget. Total Size: 22.25KB Gzipped: 8.85KB |
R: @prateekbh @gauntface @goldhand
This is another follow-up to #933, but doesn't close it yet, as there's still a TODO: to implement
importWorkboxFrom: 'local'. This PR was large enough already so I wanted to break it up further.I included a refactoring of a decent chunk of
get-manifest-entries-with-webpack.jsin this PR, because I discovered that it wasn't properly filtering out all assets based on thechunks/excludeChunkswhitelist/blacklist. That became an issue sinceimportWorkboxFrom: <chunkname>is supposed to implicitly add<chunkname>to the blacklist.This also includes a few more relevant tests, both to ensure that
importWorkboxFrom: <chunkname>works as expected, as well as ensure that whitelist/blacklist behavior works in general.Finally, I discovered that errors which occurred in the previous codebase would lead to a promise being rejected within the plugin's execution, but that failure wouldn't be bubbled up as a webpack error. This refactors the error detection so that asynchronous failures will be properly reported via the webpack callback (and includes a test for that condition).