Add Error objects, not strings, to compilation.warnings

[slavafomin]

Hello!

Library Affected:
workbox-webpack-plugin@6.1.2

I'm experiencing a situation, where the manifest, generated by the InjectManifest plugin doesn't include the main.js file (this is a main and a single entry point of my application).

I've tried to use the webpack-manifest-plugin and I can confirm that it's manifest correctly lists all the output files, including the main.js one.

I'm using latest Webpack 5.27.1.

Do you have an idea what could be causing this behavior? Thanks.

[slavafomin]

Well, the problem was with the maximumFileSizeToCacheInBytes option, which is enabled by default and applies even in development. Sadly, this behavior is not documented anywhere, even the default value for maximumFileSizeToCacheInBytes is not documented. I found it just by a lucky guess.

I would suggest to update the documentation to make this point much more clear. Also, it could be a good idea to disable this filter by default entirely. I think that developer should be the one to explicitly determine if this rule should apply. Sadly, this option can't be even disabled without specifying a big numeric value there.

[jeffposnick]

I apologize for the frustration that this caused.

Our current JSDoc to HTML generation pipeline unfortunately omits our documented default values, so while this is considered our "source of truth", it's not visible like it should be:

workbox/packages/workbox-webpack-plugin/src/inject-manifest.js

Lines 100 to 103 in 27080cc

	* @param {number} [config.maximumFileSizeToCacheInBytes=2097152] This value can be
	* used to determine the maximum size of files that will be precached. This
	* prevents you from inadvertently precaching very large files that might have
	* accidentally matched one of your patterns.

(We have plans to switch to an alternative HTML generation process as part of a larger documentation migration.)

The one mitigating point is that exclusions of entries from the precache manifest should be reported back to the webpack compilation as runtime warnings, which should in turn be visible to developers:

workbox/packages/workbox-build/src/lib/maximum-size-transform.js

Lines 19 to 21 in 27080cc

	warnings.push(`${entry.url} is ${prettyBytes(entry.size)}, and won't ` +
	`be precached. Configure maximumFileSizeToCacheInBytes to change ` +
	`this limit.`);

Is that warning what eventually led you to discover what was going on?

[raphcollective]

I also faced the exact same issue this week. I didn't get a warning while compiling, I only discovered that option because of this thread.

[jeffposnick]

Thanks for letting us know. The intention is definitely for there to be a warning bubbled up that gets reported by webpack. Let me try to reproduce the issue to see if something's preventing that from happening.

[raphcollective]

Thanks for having a look! I'm using the following packages:

        "laravel-mix": "^6.0.0-beta.17",
        "laravel-mix-workbox": "^0.1.4",
        "workbox-cacheable-response": "^6.1.2",
        "workbox-core": "^6.1.2",
        "workbox-expiration": "^6.1.2",
        "workbox-precaching": "^6.1.2",
        "workbox-recipes": "^6.1.2",
        "workbox-routing": "^6.1.2",
        "workbox-strategies": "^6.1.2",
        "workbox-webpack-plugin": "^6.1.2"

[jeffposnick]

I tried to reproduce this with a basic package.json:

"scripts": {
  "build": "webpack"
},
"devDependencies": {
  "webpack": "^5.27.2",
  "webpack-cli": "^4.5.0",
  "workbox-webpack-plugin": "^6.1.2"
}

and the webpack configuration:

const {InjectManifest} = require('workbox-webpack-plugin');
const path = require('path');

module.exports = {
  entry: './test.js',
  mode: 'development',
  output: {
    path: path.resolve(__dirname, 'build'),
    filename: 'bundle.js',
  },
  plugins: [
    new InjectManifest({
      swSrc: './sw.js',
      maximumFileSizeToCacheInBytes: 1,
    }),
  ],
};

Running the webpack-cli to build it leads to a warning logged as expected:

> webpack

asset bundle.js 1.17 KiB [emitted] (name: main)
asset sw.js 1.15 KiB [compared for emit]
./test.js 22 bytes [built] [code generated]

LOG from InjectManifest
<i> The service worker at sw.js will precache
<i>         0 URLs, totaling 0 B.

WARNING in bundle.js is 1.2 kB, and won't be precached. Configure maximumFileSizeToCacheInBytes to change this limit.

webpack 5.27.2 compiled with 1 warning in 91 ms

That's what I would expect to see, and indicates that the warnings and logs from workbox-webpack-plugin are properly reflected in the webpack compilation. I think it would give a developer enough to go on to figure out why an entry wasn't precached.

If you're using a wrapper like laravel-mix on top of webpack, perhaps that wrapper is not properly passing through warnings and log messages that originate from webpack plugins?

[raphcollective]

I agree that the warning would be enough to figure out what's going on. The issue must be coming from either mix or the plugin wrapper but I'm wondering why some warnings are showing up fine then. For example, this one is displayed without issue:

InjectManifest has been called multiple times, perhaps due to running webpack in --watch mode. The precache manifest generated after the first call may be inaccurate! Please see #1790 for more information.

Is that message using a different method to display the warning?

[jeffposnick]

That specific warning comes from:

workbox/packages/workbox-webpack-plugin/src/inject-manifest.js

Lines 277 to 285 in 27080cc

	const warningMessage = `${this.constructor.name} has been called ` +
	`multiple times, perhaps due to running webpack in --watch mode. The ` +
	`precache manifest generated after the first call may be inaccurate! ` +
	`Please see https://github.com/GoogleChrome/workbox/issues/1790 for ` +
	`more information.`;
	if (!compilation.warnings.some((warning) => warning instanceof Error &&
	warning.message === warningMessage)) {
	compilation.warnings.push(new Error(warningMessage));

The warning about the maximum file size, and any other warnings related to manifest entries, come from:

workbox/packages/workbox-webpack-plugin/src/lib/get-manifest-entries-from-compilation.js

Lines 193 to 203 in 27080cc

	const {manifestEntries, size, warnings} = await transformManifest({
	fileDetails,
	additionalManifestEntries: config.additionalManifestEntries,
	dontCacheBustURLsMatching: config.dontCacheBustURLsMatching,
	manifestTransforms: config.manifestTransforms,
	maximumFileSizeToCacheInBytes: config.maximumFileSizeToCacheInBytes,
	modifyURLPrefix: config.modifyURLPrefix,
	transformParam: compilation,
	});
	compilation.warnings = compilation.warnings.concat(warnings || []);

The main difference that I see is that we're adding Error objects to compilation.warnings in the first example, and in the second, the values added will be strings.

I can't easily reproduce the build environment you have set up, so do you want to try making the following change to your local copy of your workbox-webpack-plugin's lib/get-manifest-entries-from-compilation.js?

// Comment out the following line and add:
// compilation.warnings = compilation.warnings.concat(warnings || []); 
compilation.warnings.push(...warnings.map((warning) => new Error(warning)));

[raphcollective]

That fix did make a difference, I can now see the warnings. Maybe it is related to how this package used by laravel-mix parses the errors and warnings? It looks like it hasn't been updated in a while.

[jeffposnick]

Okay, thanks for confirming. I can put together a PR that switches that code over to wrap the strings in Error objects, after confirming that it's backwards compatible with webpack v4.40+. (I think it should be.)

[raphcollective]

That'd be great, thank you for your help and responding so quickly!

[jeffposnick]

...and now I remember that we made that change to some other parts of the codebase recently to resolve #2674, but it looks like that one line of code still was using strings directly. Sorry about that.

[slavafomin]

Hello! Thank you for a quick and detailed response.

I don't believe I saw the warning. At least not after I've figured this out by myself. The problem with warnings is that Webpack can generate a lot of messages and most developers are getting blind to it's output. I still believe that default settings should be more conservative.

[jeffposnick]

I still believe that default settings should be more conservative.

One way of looking at it, which has motivated the defaults that have been in place for five years or so now, is that accidentally forcing your users to precache, say, dozens of megabytes of image assets is not being conservative.

There's a balance we have to strike between confusing developers who are unclear on why something isn't precached (and hopefully the warning messages help here) and confusing developers who hear from angry users about how their deployed web app downloads much more data than they expected the first time they visit it.