Warn when injectManifest({runtimeCaching, navigateFallback}) is called

[jeffposnick]

Library Affected:
workbox-build

There are a few options that could be passed in to workbox-build.injectManifest() (either directly, or via workbox-cli or workbox-webpack-plugin) that don't make any sense, since in that mode, we're only replacing the contents of precache([]), and not making any other changes to the swSrc file. The ones that I can think of include:

• runtimeCaching
• navigateFallback
• navigateFallbackWhitelist

but there might be others—as part of #695 I might come across more.

Instead of silently ignoring them, leading to developer confusion when they don't have any effect, we should throw a fatal exception and explain why they shouldn't be used in the injectManifest() mode.

[addyosmani]

leading to developer confusion when they don't have any effect, we should throw a fatal exception and explain why they shouldn't be used in the injectManifest() mode.

If passing one of these options to injectManifest has no destructive side-effect, it seems like we shouldn't be penalizing developers with a fatal exception. Is there a reason you'd prefer this to warning?

[jeffposnick]

Warning would still be an improvement vs. silently ignoring them.

The issue I have with build-time warnings is that they have trouble catching the developer's attention, especially if they're mixed in with a bunch of other build output as part of a larger pipeline. With a fatal error, you'll definitely get the developer's attention, and it forces them to take action to resolve.

But we can start with console.warn() as a non-breaking-change, and then revisit making it a fatal error when it's time for the next semver major release.

[gauntface]

Have you seen any developers get confused by this?

[jeffposnick]

I haven't seen issues raised or developers complaining due to confusion.

[gauntface]

Happy to make this display warnings, can see it being useful, but I'm 👎 on throwing an error unless it's clearly a pain point for developers.

[jeffposnick]

Okay, we'll go with the warnings approach.

[whawker]

@jeffposnick can I ask why the runtimeCaching option makes no sense when injecting a manifest?

It seems to me, being able to define caching rules in my Webpack build script can be more powerful than having to define them in a swSrc template file?

My use case is, that I want to switch the domain, depending on whether I'm building for my test environment or the prod environment.

const baseDomainTemplate = '^https://{SERVICE}.' + process.env.BASE_DOMAIN;
const getDomainRegex = service => new RegExp(baseDomainTemplate.replace('{SERVICE}', service));

runtimeCaching: [
  {
    urlPattern: getDomainRegex('feeds'),
    handler: 'networkFirst'
  },
  {
    urlPattern: getDomainRegex('profiles'),
    handler: 'networkFirst'
  }
]

[jeffposnick]

The idea is that if you'd like to define your service worker's behavior in your webpack config, then using the GenerateSW plugin allows for that.

The InjectManifest plugin is intended for developers who want to "own" their service worker's behavior, but still want the precache manifest generation/integration.

[RehanSaeed]

I only want InjectManifest to add a web push ability. It would be very useful if I could use the runtimeCaching option with InjectManifest. Is there a particular reason why it has been removed? What is the alternative?

[jeffposnick]

One alternative is to use GenerateSW with options like runtimeCaching once to come up with a a base service worker file, make a copy of it, and then remove the bits that import the precache manifest. You'd then add whatever logic you want regarding push notification into to that file. Then you can pass in that service worker file as swSrc when using InjectManifest.

Another option is to use GenerateSW but include your push notification code in a separate .js file, and then use the importScripts option to effectively pull in that code into your service worker execution.