GoogleChrome/workbox

Precache manifest missing entries when recompiled via webpack --watch

lakesare opened this issue ยท 19 comments

Library Affected:
workbox-webpack-plugin

Browser & Platform:
Webpack 2.3.3, workbox-webpack-plugin 3.6.3

Issue Description:
In the webpack --watch mode, precache-manifest.hash.js file at first generates a perfect list of files, but on subsequent updates it misses some files.
For example, files generated with CopyWebpackPlugin are missing (but including copyUnmodified: true in options helps).
Another kind of files that are missing, is fontawesome-webfont.eot?hash files (importes via this process https://medium.com/@chanonroy/webpack-2-and-font-awesome-icon-importing-59df3364f35c), this I don't yet know how to fix.

Here is a similar issue for another manifest plugin: shellscape/webpack-manifest-plugin#144.

Just adding to this, for completeness, the navigate fallback file is also ejected on re-build.

Sorry for letting this drop until now.

workboox-webpack-plugin background

workbox-webpack-plugin generates its precache manifest based on the contents of the current compilation's assets, as determined in a compiler hook into the emit event. I think that's fairly standard for a webpack plugin.

webpack --watch mode behavior

After running some local experiments with --watch mode, I've found that only brand new assets, and modifications to existing assets, show up in the compilation's assets available during each subsequent emit hook invocation.

I believe some other plugins (like webpack-manifest-plugin, as referenced in shellscape/webpack-manifest-plugin#144) maintain state information about all of the previous compilation's assets and make additions or changes to hashes to that state information upon each subsequent compilation. They then create their output based on the cumulative total of all the assets that have been created across multiple compilations. (This seems to be the purpose of the seed parameter that can be passed to webpack-manifest-plugin.)

Problems with cumulative state

workbox-webpack-plugin could do something similar, and generate its precache manifest based on a cumulative list of assets across multiple compilations.

The concern that I have, though, is that there is no way for a webpack plugin to detect that a given asset has been removed following a recompilationโ€”the absence of an asset from any particular compilation can't be used as a signal that it was removed, since that scenario is identical to what would happen if it was still present but unchanged.

Unfortunately, it's pretty easy to trigger that scenario even without bringing any third-party plugins into the mix; if you're following the official guide for dynamic imports with output.chunkFilename: '[name].bundle.js', and you have code like:

// Assume this is in your main chunk:
async function load() {
  const b = await import(/* webpackChunkName: "b" */ './b.js');
  const c = await import(/* webpackChunkName: "c" */ './c.js');
}

If you remove the dynamic import for c in watch mode, that will trigger a recompilation, but the only entries provided in assets will be b and the main chunk. There's no indication that c is no longer emitted.

Possible solutions

Given that, it sounds like there's a tradeoff when in --watch mode between potentially leaving out manifest entries that should be there (the current behavior) and including entries that correspond to assets that have been deleted (if we maintained asset info across compilations, and cumulatively added to it).

Neither solution is ideal, but I think from a service worker functionality perspective, the current behavior might be preferableโ€”if you include a URL in the precache manifest list that doesn't exist, the installation process of the updated service worker will end up failing entirely. That sounds worse than the current behavior, in which the updated service worker installs but precaches less than expected. I'd love to hear folks' thoughts, though.

Additional warnings

What I plan on doing regardless is to detect when workbox-webpack-plugin has been called across multiple compilations and add a warning to the webpack compilation letting developers know not to trust the output for anything that should be considered production-ready. I will include a link that points to this (hopefully complete) description of the problem and trade-offs to give developers more context. The ultimate recommendation is that developers should only deploy a service worker that was generated by a one-shot webpack compilation.

Ugh Rock and a Hard place, Jeff. Sorry webpack leaves you so few good options.

Would plugging this specific bug make sense? Ie, on watch, manually, forcibly check for the navigateFallback, and just add that? Not perfect, but it would solve one of the worse manifestations of this bug.

So what I'd like to do for the v5 timeframe is add in the webpack compilation warning when we detect that re-compilation has occurred, letting folks know that the contents of the precache manifest might be inconsistent.

I'm going to explore a bit more to see if there's anything in the webpack internals that could give us the info we need about the complete set of assets, raise issues with the webpack maintainers if not, and in all likelihood switch things over to the cumulative asset manifest for the time being.

I don't want to block the v5 release on that work, as it's likely going to take some time. I feel like adding in the warning about inconsistently gives us some leeway to make this sort of change post-v5, since developers will have a heads-up about not relying on --watch for anything production-ready.

Is there a recommendation how to use this in dev mode (with watch or dev server)?
Turning off the plugin then, obviously results in errors since the service worker is not available.
Leaving it on emits a lot of warnings - and the feeling that something might not work as expected.

There are a few approaches:

  • Serve a no-op, empty service-worker.js file from your development web server.
  • Put the call to navigator.serviceWorker.register() behind a process.env.NODE_ENV check.
  • If you actually do want aggressive precaching in your development environment (which, honestly, I don't think is very helpful), you might be able to cobble something together to work around this issue using manifestTransform in v5. I could imagine persisting the originalManifest parameter outside of the lifetime of a given compilation, and then re-adding any missing manifest entries each time a re-compilation occurs. You'd risk re-adding manifest entries for assets that have been deleted, though, but I guess it's a development environment and you could deal with some inaccuracy.

I'd personally go with one of the first two options, as I think testing your service worker is best done via a "staging" rather than live-reload "development" build.

I'd personally go with one of the first two options, as I think testing your service worker is best done via a "staging" rather than live-reload "development" build.

I don't know if it's just because our case is unusual, but this would be very painful for my team. We recently added a Workbox service worker to a legacy application, and getting it right was largely a process of trial and error. Our non-live-reload Webpack build times take minutes to complete, whereas live-reload lets us fail fast and move on.

Basically, sometimes what you're developing is the service worker itself, or requires the service worker to be present to function (e.g. Notifications on Android Chrome).

We also value having the service worker in live-reload situations because of the concept of dev/prod parity. If developers are always working without the service worker running, they will probably miss issues that can and do arise once a service worker is in play.

FWIW, we went with option 3 above, but it doesn't save us from getting spammed with redundant warning messages over and over:

Screen Shot 2019-11-15 at 9 46 46 am

P.S. I have some experience writing Webpack plugins, so I do sympathise with how challenging it could be to find a "real" fix for this. I just wanted to plead my case ๐Ÿ™

@jeffposnick any new solutions since then?

There aren't any new solutions at this time. The warning message that gets logged in v5 will hopefully let folks know about the issue while doing local development.

My temporary solution. So I can make development lighter:

const withPWA = require('next-pwa');

const settings = {
  env: {
  },
  devIndicators: {
    autoPrerender: false,
  },
  pwa: {
    dest: 'public',
  },
};

module.exports = process.env.NODE_ENV === 'development' ? settings : withPWA(settings);

A similar approach as per @devarthurribeiro, but through the disable option:

const withPWA = require('next-pwa')
const prod = process.env.NODE_ENV === 'production'

module.exports = withPlugins([[withSaas], [withPWA], [withImages]], {
(...)
  pwa: {
    disable: prod ? false : true,
    dest: 'public'
  },
(...)

I don't think there's anything more we can do here, other than log the warning on recompilation that we've implemented.

Could you make the warning only display once when multiple copies of workbox-webpack-plugin are in use? For white-label software with 7 brands, we get 7 warnings in the console every time a re-build occurs.

@ezzatron that should be doable.

As specified in https://www.npmjs.com/package/next-pwa?activeTab=readme#configuration

You can also do:

const withPWA = require('next-pwa')
 
module.exports = withPWA({
  pwa: {
    disable: process.env.NODE_ENV === 'development',
    register: true,
    scope: '/app',
    sw: 'service-worker.js',
    //...
  }
})

As specified in https://www.npmjs.com/package/next-pwa?activeTab=readme#configuration

You can also do:

const withPWA = require('next-pwa')
 
module.exports = withPWA({
  pwa: {
    disable: process.env.NODE_ENV === 'development',
    register: true,
    scope: '/app',
    sw: 'service-worker.js',
    //...
  }
})

I don't get it... Is there a solution that doesn't require using next.js? Or does next-pwa not require using next.js?

As specified in https://www.npmjs.com/package/next-pwa?activeTab=readme#configuration
You can also do:

const withPWA = require('next-pwa')
 
module.exports = withPWA({
  pwa: {
    disable: process.env.NODE_ENV === 'development',
    register: true,
    scope: '/app',
    sw: 'service-worker.js',
    //...
  }
})

I don't get it... Is there a solution that doesn't require using next.js? Or does next-pwa not require using next.js?

whatever your pipeline, simply don't use workbox when process.env.NODE_ENV === 'development' ๐Ÿ˜Œ

lourd commented

My team has a case where we want to generate and register the service worker in development. This is because we have caching set up in our service-worker for some very large assets that are not a part of our webpack build, and we also want to be able to test and receive push notifications in local development.

If that's also your case, I figured out this hacky workaround:

const nextConfig = {
  webpack(config, options) {
    if (!options.isServer) {
      const workboxPlugin = new InjectManifest({
        swSrc: "./src/service-worker/index.ts",
        swDest: "../public/service-worker.js",
        // In dev, exclude everything.
        // This avoids irrelevant warnings about chunks being too large for caching.
        // In non-dev, use the default `exclude` option, don't override.
        ...(options.dev ? { exclude: [/./] } : undefined),
      })
      if (options.dev) {
        // Suppress the "InjectManifest has been called multiple times" warning by reaching into
        // the private properties of the plugin and making sure it never ends up in the state
        // where it makes that warning.
        // https://github.com/GoogleChrome/workbox/blob/v6/packages/workbox-webpack-plugin/src/inject-manifest.ts#L260-L282
        Object.defineProperty(workboxPlugin, "alreadyCalled", {
          get() {
            return false
          },
          set() {
            // do nothing; the internals try to set it to true, which then results in a warning
            // on the next run of webpack.
          },
        })
      }
      config.plugins.push(workboxPlugin)
    }
    return config
  }
}

You may also need to set self.__WB_DISABLE_DEV_LOGS = true in the service-worker source to suppress extraneous warnings in the browser console.

My team has a case where we want to generate and register the service worker in development. This is because we have caching set up in our service-worker for some very large assets that are not a part of our webpack build, and we also want to be able to test and receive push notifications in local development.

If that's also your case, I figured out this hacky workaround:

const nextConfig = {
  webpack(config, options) {
    if (!options.isServer) {
      const workboxPlugin = new InjectManifest({
        swSrc: "./src/service-worker/index.ts",
        swDest: "../public/service-worker.js",
        // In dev, exclude everything.
        // This avoids irrelevant warnings about chunks being too large for caching.
        // In non-dev, use the default `exclude` option, don't override.
        ...(options.dev ? { exclude: [/./] } : undefined),
      })
      if (options.dev) {
        // Suppress the "InjectManifest has been called multiple times" warning by reaching into
        // the private properties of the plugin and making sure it never ends up in the state
        // where it makes that warning.
        // https://github.com/GoogleChrome/workbox/blob/v6/packages/workbox-webpack-plugin/src/inject-manifest.ts#L260-L282
        Object.defineProperty(workboxPlugin, "alreadyCalled", {
          get() {
            return false
          },
          set() {
            // do nothing; the internals try to set it to true, which then results in a warning
            // on the next run of webpack.
          },
        })
      }
      config.plugins.push(workboxPlugin)
    }
    return config
  }
}

You may also need to set self.__WB_DISABLE_DEV_LOGS = true in the service-worker source to suppress extraneous warnings in the browser console.

based on this workaround, i just added this to my webpack config file, and the warning gone <3

module.exports = (env) => {
	if (env !== 'production') {
		Object.defineProperty(workboxPlugin, 'alreadyCalled', {
			get() {
				return false
			},
			set() {}
		})
	}
	...
}