adding swSrc as webpack asset capability #1763
Conversation
There was a problem hiding this comment.
Hardcoding 'sw.js' is not a good idea, as plenty of folks use alternative names for their service worker file. You'd need to take this in as a config parameter.
...and since you need a config parameter, maybe it makes sense to reuse swSrc for this purpose? You can have a check like:
let originalSWString;
if (this.config.swSrc in compilation.assets) {
originalSWString = compilation.assets[this.config.swSrc].source();
} else {
originalSWString = await readFileWrapper(readFile, this.config.swSrc);
}Of course, that's basically magic overloading of the existing option, and maybe it doesn't feel very webpacky. In which case, I think we'd need a new config option.
|
woops sry, I copied it from the project which caused this feature request, but I feel |
|
I am closing this as swSrc should never be a webpack asset. The compilation generating the assets will mostly have webpack target as "window" but the any service worker file being produced needs to have target as worker. Since the service worker and rest of the assets cannot share the same compilation, the plugin can only have access to either the list of assets or the service worker file. |
|
Sorry, I did discover a valid use case for this now: "childCompiler" Since a child compiler can generate swSrc in the same compilation with a different target, hence this change will be required |
|
I'd love to get @developit's take on how this fits in with the other things he's looking into. |
|
@developit bumping this thread for your review, as the release of this feature is blocking integration in preact-cli. Which btw also needs to be reviewed by you 😂 |
|
@developit, can you take a look at this PR and let us know if it conflicts with any of your future plans for the webpack plugin. This is blocking the preact cli integration, but I wouldn't want to merge this only to change the behavior in v5. |
|
Honestly what @prateekbh has set up in his Preact CLI PR is super close to what was in my head. The only difference would be that I'd like to bundle via Webpack and import workbox as modules, which I think we can still add independently in v5? @prateekbh do you think it would be prohibitive to require that a custom Service Worker defined via |
Or perhaps (whether in this version or a future version), there could be an option to use either |
|
@philipwalton yup. writing up an RFC right now. |
|
@developit is there a good reason to make it compulsory to not use a global workbox via importScript? |
|
Nope, I meant leave it as an exercise for the developer. They can importScripts(). |
|
I guess it'll be somewhat restrictive to say that in order to use |
|
It'd also be a little counterintuitive, since Service Worker doesn't natively support Modules, but does support importScripts(). I am a fan of adding "futurey" Modules support, but it seems worth keeping the option to use current stuff there too.
|
There was a problem hiding this comment.
It would be lovely if you could add a test case to https://github.com/GoogleChrome/workbox/blob/master/test/workbox-webpack-plugin/node/inject-manifest.js
| * this is the case where source is also a webpack asset. | ||
| */ | ||
| if (compilation.assets[relSwDest]) { | ||
| delete compilation.assets[relSwDest]; |
There was a problem hiding this comment.
Line 159 assigns a value to compilation.assets[relSwDest], which should overwrite anything that's already there, so I'm not clear on what this delete accomplishes. Can you clarify?
There was a problem hiding this comment.
I saw that simply overriding was somehow not overriding the content, so I added an additional delete statement which did the trick
There was a problem hiding this comment.
That feels kind of magic. @developit, have you ever seen anything like that?
(Also, I'm not sure if there's a reason to wrap this in an if(), since the delete will be a no-op if the property doesn't exist.)
There was a problem hiding this comment.
Weird, I've never had to do this. @prateekbh any chance it was some other issue? I can't think of why this would happen.
There was a problem hiding this comment.
well yeah it works now.. dont know what was the problem earlier..
…rc-as-webpack-asset
…e/workbox into src-as-webpack-asset
|
@jeffposnick added the tests |
PR-Bot Size PluginChanged File Sizes
New Files
All File SizesView Table
Workbox Aggregate Size Plugin8.84KB gzip'ed (59% of limit) |
swSrcas a webpack asset filename