Make it easier to use a custom Webpack SW bundle with the plugin

[jeffposnick]

@anilanar brought up the following in #808 (comment). (Please correct me if there's any nuance missing.)

I assumed a raw service-worker.js could look as follows:

import WorkboxSW from 'workbox-sw';
const workboxSW = new WorkboxSW();

const modifyManifest = manifest => { /* do something with manifest */ }
workboxSW.precache(modifyManifest(self.__precacheManifest));

After building with webpack and processing with workbox-webpack-plugin:

importScripts('file-manifest.{hash}.js');
(function () {
      /* webpack generated code here */
})();

Ideally, users can opt to use workbox-precaching package instead of the whole workbox-sw when they don't need anything else than simple precaching.

[gauntface]

@jeffposnick this can be closed given the workbox-sw changes no just being a loader

[jeffposnick]

I'd like to leave this open, as there are still some ergonomic issues around automatically opting-out of using the CDN copy of Workbox if we see a signal that a custom build is being used.

[gauntface]

@jeffposnick could you provide a little more info.

My understanding is that to opt out of the CDN from webpack the following would happen:

• Developer would inject the manifest meaning the plugin would just inject and do nothing with respect to using the CDN vs local files.
  • If the developer decides NOT to use the CDN, they'd get a copy of the files from Github, workbox-cli or workbox-build and then set up workbox-sw accordingly (https://developers.google.com/web/tools/workbox/modules/workbox-sw#using_local_workbox_files_instead_of_cdn)

[jeffposnick]

There's not a huge amount of code that needs to be changed, but there's currently some awkwardness due to assumptions being made in the webpack plugin, roughly around

workbox/packages/workbox-webpack-plugin/src/index.js

Lines 123 to 128 in f743aa9

	this.config.importScripts = (this.config.importScripts || []).concat([
	getModuleUrl('workbox-sw'),
	manifestFilename,
	]);
	const serviceWorker = await generateOrCopySW(this.config, swSrc);

The webpack plugin currently uses the presence/absence of swSrc to toggle between the equivalents of injectManifest and genereateSW modes. My rough summary of where things stands/what should probably change:

• When in injectManifest mode, we need to stop automatically import workbox-sw.js from the CDN.
• In either mode, I guess explicitly setting importWorkboxFromCDN should allow developers to override the defaults.
• I'm not sure whether developers are more likely to want to specify own custom Workbox bundle via importScripts vs. generating the entire top-level service worker file and just using importScripts to pull in a manifest generated by the plugin. Depending on those use cases, there may be additional work needed to change defaults.

[gauntface]

The only thing I ask is that we have consistency across behaviours in webpack, workbox-build and workbox cli.

[anilanar]

I personally like symmetries between different parts of a software and I think the following flow is not far-fetched.

1. Copy paste a typical webpack configuration that sets up chunkhashes, vendor and main chunks, js loaders and uglifiers etc.
2. Use a plugin (let's call it SwWebpackPlugin) to generate sw.js that would import scripts (analogous to HtmlWebpackPlugin). Workbox library is not involved yet. sw.js would look as follows:

importScripts('./path/to/vendor.{hash}.js');
importScripts('./path/to/main.{hash}.js');

3. Later on they decide to use workbox: npm install workbox-precache workbox-webpack-plugin
4. In their main js file, they do import precache from 'workbox-precache; precache(self.__precacheManifest);
5. Configure workbox-webpack-plugin. It automatically hooks into a SwWebpackPlugin event and modifies its output as follows:

importScripts('./path/to/manifest.{hash}.js');
importScripts('./path/to/vendor.{hash}.js');
importScripts('./path/to/main.{hash}.js');

I think this scenario is viable and more familiar to devs.

[gauntface]

I like the sound of this proposal and I'm thinking that for injectManifest in workbox build and CLI, they could follow this pattern of adding an importScripts() to a service worker with a revision file name for the manifest and it defines a specific variable name (in this case self.__precacheManifest).

All of this SGTM, but I do recall me and @jeffposnick discussed this approach (or something similar) around adding the importScript() but not being 100% on it, I think it may be an issue of not working on a dev env where the manifest may not be in the build process but it would be trivial for a dev to get around this by checking for self.__precacheManifest.

[johnnyreilly]

As discussed in the closed linked issue, importWorkboxFromCDN: false doesn't seem to be a supported option for the Workbox 3 webpack plugin. I'd really like to push for this to be included as an option. I'm afraid, based on my own experience, many organisations are likely to resist using Workbox 3.0 if it ties them to a CDN they don't control.

FWIW I'm not taking a view on this myself, but I'd like to be in a position to use Workbox 3 with clients of mine. I have a feeling that that may be a common option.

I rather think that it might be better for CDN usage to be opt-in rather than default behaviour.

I just noticed @gauntface's comment:

If the developer decides NOT to use the CDN, they'd get a copy of the files from Github, workbox-cli or workbox-build and then set up workbox-sw accordingly (https://developers.google.com/web/tools/workbox/modules/workbox-sw#using_local_workbox_files_instead_of_cdn)

I do think that ideally, not using the CDN should just work ™️ . i.e. As it does with Workbox 2.0. Going on the link @gauntface shared it sounded like opting out of the ceremony not as simple as toggling importWorkboxFromCDN to false.

Would you consider taking a look at this? One of the things I've liked so much about Workbox so far is that it's easy to get going with. From the sounds of it Workbox 3 is going to be harder to get going with if you're not going to be using the CDN. If so I think that's a real shame.

[gauntface]

@johnnyreilly it's not going to get harder (this was an issue with V2).

Sadly the docs for build and webpack and just reference docs and are lacking pretty badly but Jeff is working on some of that stuff as I type this.

My understanding from the source code is that:

1.) If you are generating a service worker, i.e. you have no swSrc, and you set importWorkboxFromCDN: false, you should be getting a local copy of all workbox files.

2.) If you DO set a swSrc then importWorkboxFromCDN should do nothing - and my understanding is that it should throw.

[johnnyreilly]

@gauntface - thanks for the clarification. That sounds perfect!

[jeffposnick]

Having let the feedback here percolate a bit, I wanted to move forward with a summary and proposal.

Problem

There are three equally valid sources that a developer using the workbox-webpack-plugin might want to use when importing the Workbox runtime code:

1. The CDN URLs.
2. A local copy of the full bundles, inside a versioned subdirectory that Workbox creates for you.
3. A custom bundle of the Workbox runtime code created by webpack and referred to as a named chunk.

Choosing between 1. and 2. is something that developers using the workbox-build and workbox-cli tools also have to do, and we currently use the boolean importWorkboxFromCDN to toggle between those modes. However, booleans obviously don't map well to toggling between three choices, especially when one of those choices needs additional state information (the name of the chunk with the Workbox bundle).

Proposal

• Disallow using importWorkboxFromCDN within workbox-webpack-plugin.
• Create a new config parameter, importWorkboxFrom, which takes a string value.
  • If passed the string 'cdn', then use the CDN URL for importScripts().
  • If passed the string 'local', then make a local copy of the Workbox libraries into a versioned directory, and use that local URL for importScripts().
  • If passed any other string value, assume it's a chunk name, and pass the URL corresponding to the output of that chunk to importScripts(). Additionally, add that chunk name to the excludeChunks list, to make sure it's not accidentally included in the precache manifest.

Open Questions

• Should we keep importWorkboxFromCDN as-is for the other build tools, or standardize on importWorkboxFrom across all of them, but only allow for values of 'cdn' and 'local'? (My vote is to standardize on importWorkboxFrom and default it to 'cdn'.)
• Is using a string value the right way to configure this? Using 'local' and 'cdn' both feel magic and potentially error prone, though the joi validation should help steer people using workbox-build and workbox-cli in the right direction.

CC: @johnnyreilly @goldhand @gauntface @anilanar for thoughts.

[gauntface]

Could we have importWorkboxFrom ['cdn', 'local', null] as valid options for all the build tools with a default of 'cdn' and kill the importWorkboxFromCDN. Main reason is that it forces a choice, provides convenience for CDN and local and has the escape hatch with null (i.e. do nothing).

Regarding the string, I think it's fine, but we should provide a logical error message if a invalid value, whether its joi or us that logs the message I'm not sure.

Reasoning: I would like us to bring things together again, doing different actions across these tools is a bad route to take.

[jeffposnick]

I'm cool with using null as a valid option to disable any sort of automatic behavior across all of the tools.

But, to clarify, are you fine with allowing a chunk name as a valid importWorkboxFrom value for the workbox-webpack-plugin use case?

[johnnyreilly]

Could we have importWorkboxFrom ['cdn', 'local', null] as valid options for all the build tools with a default of 'cdn' and kill the importWorkboxFromCDN.

I think that's a good shout.

But, to clarify, are you fine with allowing a chunk name as a valid importWorkboxFrom value for the workbox-webpack-plugin use case?

It's not a feature I plan to use but I've no objection to it. Though what if there's a chunk name of 'cdn' or 'local'? I guess the advice is "change the chunk name"?

I'm cool with using null as a valid option to disable any sort of automatic behavior across all of the tools.

Could you clarify what this means exactly? What would be the point of doing that / the impact of doing that?

Also, supplying an option of importWorkboxFrom: null seems a little non-descriptive. I can imagine this being an option that people aren't aware of / don't understand. Would a value of 'disable' or something be more meaningful perhaps?

[gauntface]

@jeffposnick I honestly don't understand what a "chunk name" means so I have no opinion on this.

👍 'disable' instead of null.

Use case is largely for - I want to create a single bundle of my code and it'll be all minified but I want to inject a manifest and I'll provide a custom regex injection etc.

[johnnyreilly]

Thanks for sharing @gauntface

I want to inject a manifest and I'll provide a custom regex injection etc.

By manifest I'm assuming you mean one of these?:

self.__precacheManifest = [
  {
    "revision": "31d9b4d3ded3e6cef2e7",
    "url": "print.bundle.js"
  },
  {
    "revision": "f3e69d5c4f5bc039cf6072032661e25d",
    "url": "index.html"
  },
  {
    "revision": "ee1f88b1ae206056267a",
    "url": "app.bundle.js"
  }
];

I'll provide a custom regex injection etc.

Could you tell me what you mean by this please? Feels like something I should understand but I don't

[gauntface]

in workbox-build at the moment we don't have self.__precacheManifest (although I wish be did), instead it looks for workbox.precache.precacheAndRoute([]) and injects the array of files there (replacing []). We've allowed the ability to custom this regex to inject else where.

It's fair from ideal in v3 and one area I'd like to re-think in V4, but now it's where we are.

[johnnyreilly]

Ah I see - thanks

[jeffposnick]

I think we have consensus. I'm going to put together a PR that:

• Removes importWorkboxFromCDN from all build tools.
• Add importWorkboxFrom => 'cdn'|'local'|'disabled' as a supported option in all build tools. It will always default to 'cdn', and 'disabled' (which I prefer to 'disable') will turn it off.
  • Using it webpack plugin will also allow you to pass in an arbitrary string, where it will be interpreted as a chunk name.

[elf-pavlik]

@jeffposnick you suggested to continue #802 here, with you proposal above it seems that one can only import single chunk (the workbox) but can't import any other chunks.
I will need to share some of the webpack chunks between main application and the service worker, probably I should create separate issue for it?

I honestly don't understand what a "chunk name" means so I have no opinion on this.

@gauntface you might want to take a look at

• https://webpack.js.org/guides/code-splitting/
• https://github.com/goldhand/sw-precache-webpack-plugin#configuration

  • importScripts: [Array<String|Object>]
    • When importScripts array item is a String:
      • Converts to object format { filename: '<publicPath>/my-script.js'}
    • When importScripts array item is an Object:
      • Looks for chunkName property.
      • Looks for filename property.
      • If a chunkName is specified, it will override the accompanied value for filename.

[jeffposnick]

I will need to share some of the webpack chunks between main application and the service worker, probably I should create separate issue for it?

That sounds like it should be tracked separately, yes.

Sorry to ping-pong from issue to issue, but I think what you're describing might require a completely new config parameter, like importChunks, which would end up being treated separately from the other use cases.