Support importWorkboxFrom: 'local' in the InjectManifest webpack plugin #1290
Conversation
| @@ -80,10 +80,13 @@ describe(`[workbox-webpack-plugin] GenerateSW (End to End)`, function() { | |||
| const compiler = webpack(config); | |||
| compiler.run((webpackError) => { | |||
| if (webpackError) { | |||
| expect(webpackError.message.includes('importWorkboxFrom')); | |||
| done(); | |||
| if (webpackError.message.includes('importWorkboxFrom')) { | |||
There was a problem hiding this comment.
FYI, in the course of writing this PR, I discovered that this test pattern wasn't actually working as it was initially implemented: it should have been expect(webpackError.message.includes('importWorkboxFrom')).to.be.true, although that would then cause an unhandled promise rejection rather than a clean error to report back to mocha.
I took the opportunity to clean up this test along with the corresponding test in the inject-manifest.js suite so that it properly detects the failure and reports it using the done() callback.
| const postInjectionSWString = `importScripts(${importScriptsString}); | ||
| const setConfigString = modulePathPrefix | ||
| ? `workbox.setConfig({modulePathPrefix: ` + | ||
| `${JSON.stringify(modulePathPrefix)}});` |
There was a problem hiding this comment.
Do you think JSON.stringify is necessary?
There was a problem hiding this comment.
It's safer than wrapping the value in either "..." or '...', just on the off chance modulePathPrefix contains any quote characters. (Not that it should, but still.)
JSON.stringify() is guaranteed to return a properly escaped string.
| }); | ||
| }); | ||
|
|
||
| it(`should support setting importWorkboxFrom to 'local'`, function(done) { |
There was a problem hiding this comment.
It'd be cool to include a test where Webpack publicPath is specified
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 154% of our max size budget. Total Size: 22.52KB Gzipped: 9.01KB |
|
After testing out What do you guys think of a different implementation on copying on disk versus Webpack compilation? |
|
Rather than discussing this on a merged PR, I created #1338 |
R: @gauntface
CC: @raejin
Fixes #1260
This adds
importWorkboxFrom: 'local'support to theInjectManifestwebpack plugin. (It's still not supported ininjectManifestmode of theworkbox-buildorworkbox-cli.)Assuming this PR is merged, I'll need to update https://github.com/google/WebFundamentals to reflect the new config.