From 515f66f2ac2779dd08aae589d06268d5974cfc79 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Wed, 23 Oct 2019 15:54:38 -0400 Subject: [PATCH 1/4] Unrevisioned precaching warning. --- .../additional-manifest-entries-transform.js | 4 +++ packages/workbox-build/src/lib/errors.js | 12 +++++---- .../src/PrecacheController.ts | 25 +++++++++++++++++++ test/all/node/test-prod-builds.mjs | 4 ++- .../additional-manifest-entries-transform.js | 19 +++++++------- 5 files changed, 49 insertions(+), 15 deletions(-) diff --git a/packages/workbox-build/src/lib/additional-manifest-entries-transform.js b/packages/workbox-build/src/lib/additional-manifest-entries-transform.js index aff04d9e9..57d474761 100644 --- a/packages/workbox-build/src/lib/additional-manifest-entries-transform.js +++ b/packages/workbox-build/src/lib/additional-manifest-entries-transform.js @@ -14,8 +14,12 @@ module.exports = (additionalManifestEntries) => { const stringEntries = new Set(); for (const additionalEntry of additionalManifestEntries) { + // Warn about either a string or an object that lacks a precache property. + // (An object with a revision property set to null is okay.) if (typeof additionalEntry === 'string') { stringEntries.add(additionalEntry); + } else if (additionalEntry && additionalEntry.revision === undefined) { + stringEntries.add(additionalEntry.url); } manifest.push(additionalEntry); diff --git a/packages/workbox-build/src/lib/errors.js b/packages/workbox-build/src/lib/errors.js index f9c07e0f3..6ccddc045 100644 --- a/packages/workbox-build/src/lib/errors.js +++ b/packages/workbox-build/src/lib/errors.js @@ -105,12 +105,14 @@ module.exports = { 'bad-manifest-transforms-return-value': ol`The return value from a manifestTransform should be an object with 'manifest' and optionally 'warnings' properties.`, - 'string-entry-warning': ol`Some URLs were passed to additionalManifestEntries - as strings. Please ensure that these URLs contain versioning info - (e.g. 'https://example.com/v1.0/index.js'), as Workbox cannot maintain - its own revision info for them. To disable this warning, pass in an + 'string-entry-warning': ol`Some items were passed to additionalManifestEntries + without revisioning info. Please ensure these URLs contain versioning info + (e.g. 'https://example.com/v1.0/index.js'), as Workbox will not update them + after they are initially precached. To disable this warning, pass in an object with a 'url' property, and a 'revision' property set to null - (e.g. {url: 'https://example.com/v1.0/index.js', revision: null}).`, + (e.g. {url: 'https://example.com/v1.0/index.js', revision: null}). In a + future release, using revision: null will be required. Learn + more at http://bit.ly/wb-precache`, 'no-manifest-entries-or-runtime-caching': ol`Couldn't find configuration for either precaching or runtime caching. Please ensure that the various glob options are set to match one or more files, and/or configure the diff --git a/packages/workbox-precaching/src/PrecacheController.ts b/packages/workbox-precaching/src/PrecacheController.ts index 999ab59de..bbb8e099d 100644 --- a/packages/workbox-precaching/src/PrecacheController.ts +++ b/packages/workbox-precaching/src/PrecacheController.ts @@ -66,7 +66,15 @@ class PrecacheController { }); } + const urlsToWarnAbout: string[] = []; for (const entry of entries) { + // See https://github.com/GoogleChrome/workbox/issues/2259 + if (typeof entry === 'string') { + urlsToWarnAbout.push(entry); + } else if (entry && entry.revision === undefined) { + urlsToWarnAbout.push(entry.url); + } + const {cacheKey, url} = createCacheKey(entry); const cacheMode = (typeof entry !== 'string' && entry.revision) ? 'reload' : 'default'; @@ -91,6 +99,23 @@ class PrecacheController { this._urlsToCacheKeys.set(url, cacheKey); this._urlsToCacheModes.set(url, cacheMode); + + const warningMessage = `Workbox is precaching URLs without revision ` + + `info: ${urlsToWarnAbout.join(', ')}\nOnce precached, the responses ` + + `will never be updated.\nThis is only safe for URLs which ` + + `contain versioning info (e.g. /v1.0/app.js or /app.1234.js, not ` + + `/index.html).\nIf your URLs do contain revision info, pass in ` + + `a object with a 'url' property and a 'revision' property set to ` + + `null (e.g. {url: '/v1.0/index.js', revision: null}).\nIn a future ` + + `release, using revision: null will be required.\nLearn more at ` + + `http://bit.ly/wb-precache`; + if (process.env.NODE_ENV === 'production') { + // Use console directly to display this warning without bloating bundle + // sizes by pulling in all of the logger codebase in production. + console.warn(warningMessage); + } else { + logger.warn(warningMessage); + } } } diff --git a/test/all/node/test-prod-builds.mjs b/test/all/node/test-prod-builds.mjs index 13f1edee4..49d887a3c 100644 --- a/test/all/node/test-prod-builds.mjs +++ b/test/all/node/test-prod-builds.mjs @@ -25,7 +25,9 @@ describe(`[all] prod builds`, function() { const invalidFiles = []; buildFiles.forEach((filePath) => { const fileContents = fs.readFileSync(filePath).toString(); - if (fileContents.indexOf(`console`) > -1 || + if ((fileContents.indexOf(`console`) > -1 && + // See https://github.com/GoogleChrome/workbox/issues/2259 + !filePath.includes('workbox-precaching')) || fileContents.indexOf(`%cworkbox`) > -1) { invalidFiles.push(filePath); } diff --git a/test/workbox-build/node/lib/additional-manifest-entries-transform.js b/test/workbox-build/node/lib/additional-manifest-entries-transform.js index 9d4bffa40..72b611577 100644 --- a/test/workbox-build/node/lib/additional-manifest-entries-transform.js +++ b/test/workbox-build/node/lib/additional-manifest-entries-transform.js @@ -15,34 +15,35 @@ describe(`[workbox-build] lib/additional-manifest-entries-transform.js`, functio function getManifest() { return [{ url: '/first', + revision: null, }]; } it(`should not make any changes when additionalManifestEntries is empty`, function() { const transform = additionalManifestEntriesTransform([]); expect(transform(getManifest())).to.eql({ - manifest: [{url: '/first'}], + manifest: [{url: '/first', revision: null}], warnings: [], }); }); it(`should add the additionalManifestEntries to the end of the existing manifest`, function() { const transform = additionalManifestEntriesTransform([ - {url: '/second'}, - {url: '/third'}, + {url: '/second', revision: null}, + {url: '/third', revision: null}, ]); expect(transform(getManifest())).to.eql({ manifest: [ - {url: '/first'}, - {url: '/second'}, - {url: '/third'}, + {url: '/first', revision: null}, + {url: '/second', revision: null}, + {url: '/third', revision: null}, ], warnings: [], }); }); - it(`should return a warning, along with the modified manifest, when additionalManifestEntries contains a string`, function() { + it(`should return a warning, along with the modified manifest, when additionalManifestEntries contains a string or an entry without revision`, function() { const transform = additionalManifestEntriesTransform([ '/second', {url: '/third'}, @@ -50,11 +51,11 @@ describe(`[workbox-build] lib/additional-manifest-entries-transform.js`, functio expect(transform(getManifest())).to.eql({ manifest: [ - {url: '/first'}, + {url: '/first', revision: null}, '/second', {url: '/third'}, ], - warnings: [errors['string-entry-warning'] + '\n - /second\n'], + warnings: [errors['string-entry-warning'] + '\n - /second\n - /third\n'], }); }); }); From fe8a6cb8bfe28f412156d60baf0a37dccd87b892 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Wed, 23 Oct 2019 16:05:14 -0400 Subject: [PATCH 2/4] https --- packages/workbox-build/src/lib/errors.js | 2 +- packages/workbox-precaching/src/PrecacheController.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/workbox-build/src/lib/errors.js b/packages/workbox-build/src/lib/errors.js index 6ccddc045..129e269b2 100644 --- a/packages/workbox-build/src/lib/errors.js +++ b/packages/workbox-build/src/lib/errors.js @@ -112,7 +112,7 @@ module.exports = { object with a 'url' property, and a 'revision' property set to null (e.g. {url: 'https://example.com/v1.0/index.js', revision: null}). In a future release, using revision: null will be required. Learn - more at http://bit.ly/wb-precache`, + more at https://bit.ly/wb-precache`, 'no-manifest-entries-or-runtime-caching': ol`Couldn't find configuration for either precaching or runtime caching. Please ensure that the various glob options are set to match one or more files, and/or configure the diff --git a/packages/workbox-precaching/src/PrecacheController.ts b/packages/workbox-precaching/src/PrecacheController.ts index bbb8e099d..218f75c6c 100644 --- a/packages/workbox-precaching/src/PrecacheController.ts +++ b/packages/workbox-precaching/src/PrecacheController.ts @@ -108,7 +108,7 @@ class PrecacheController { `a object with a 'url' property and a 'revision' property set to ` + `null (e.g. {url: '/v1.0/index.js', revision: null}).\nIn a future ` + `release, using revision: null will be required.\nLearn more at ` + - `http://bit.ly/wb-precache`; + `https://bit.ly/wb-precache`; if (process.env.NODE_ENV === 'production') { // Use console directly to display this warning without bloating bundle // sizes by pulling in all of the logger codebase in production. From e0572ff45b4d02ae460d6a569d9dead20e88a4c0 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Wed, 23 Oct 2019 16:26:45 -0400 Subject: [PATCH 3/4] Another test + fix --- .../src/PrecacheController.ts | 32 ++++++++++--------- .../sw/test-PrecacheController.mjs | 25 +++++++++++++++ 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/packages/workbox-precaching/src/PrecacheController.ts b/packages/workbox-precaching/src/PrecacheController.ts index 218f75c6c..ec627342a 100644 --- a/packages/workbox-precaching/src/PrecacheController.ts +++ b/packages/workbox-precaching/src/PrecacheController.ts @@ -100,21 +100,23 @@ class PrecacheController { this._urlsToCacheKeys.set(url, cacheKey); this._urlsToCacheModes.set(url, cacheMode); - const warningMessage = `Workbox is precaching URLs without revision ` + - `info: ${urlsToWarnAbout.join(', ')}\nOnce precached, the responses ` + - `will never be updated.\nThis is only safe for URLs which ` + - `contain versioning info (e.g. /v1.0/app.js or /app.1234.js, not ` + - `/index.html).\nIf your URLs do contain revision info, pass in ` + - `a object with a 'url' property and a 'revision' property set to ` + - `null (e.g. {url: '/v1.0/index.js', revision: null}).\nIn a future ` + - `release, using revision: null will be required.\nLearn more at ` + - `https://bit.ly/wb-precache`; - if (process.env.NODE_ENV === 'production') { - // Use console directly to display this warning without bloating bundle - // sizes by pulling in all of the logger codebase in production. - console.warn(warningMessage); - } else { - logger.warn(warningMessage); + if (urlsToWarnAbout.length > 0) { + const warningMessage = `Workbox is precaching URLs without revision ` + + `info: ${urlsToWarnAbout.join(', ')}\nOnce precached, they ` + + `will never be updated.\nThis is only safe for URLs which ` + + `contain versioning info (e.g. /v1.0/app.js or /app.1234.js, not ` + + `/index.html).\nIf your URLs do contain revision info, pass in ` + + `a object with a 'url' property and a 'revision' property set to ` + + `null (e.g. {url: '/v1.0/index.js', revision: null}).\nIn a future ` + + `release, using revision: null will be required.\nLearn more at ` + + `https://bit.ly/wb-precache`; + if (process.env.NODE_ENV === 'production') { + // Use console directly to display this warning without bloating + // bundle sizes by pulling in all of the logger codebase in prod. + console.warn(warningMessage); + } else { + logger.warn(warningMessage); + } } } } diff --git a/test/workbox-precaching/sw/test-PrecacheController.mjs b/test/workbox-precaching/sw/test-PrecacheController.mjs index 3d64f943b..c68024518 100644 --- a/test/workbox-precaching/sw/test-PrecacheController.mjs +++ b/test/workbox-precaching/sw/test-PrecacheController.mjs @@ -155,6 +155,31 @@ describe(`PrecacheController`, function() { }); }); + it(`should log a warning unless an entry has a revision property`, function() { + const logObject = process.env.NODE_ENV === 'production' ? console : logger; + const warnStub = sandbox.stub(logObject, 'warn'); + + const precacheController = new PrecacheController(); + + precacheController.addToCacheList([ + '/should-warn', + ]); + expect(warnStub.calledOnce).to.be.true; + warnStub.resetHistory(); + + precacheController.addToCacheList([ + {url: '/also-should-warn'}, + ]); + expect(warnStub.calledOnce).to.be.true; + warnStub.resetHistory(); + + precacheController.addToCacheList([ + {url: '/should-not-warn', revision: null}, + {url: '/also-should-not-warn', revision: '1234abcd'}, + ]); + expect(warnStub.notCalled).to.be.true; + }); + it(`should add url + revision objects to cache list`, async function() { const precacheController = new PrecacheController(); From feee593238c6df3df44405fa78f8a6dfa4cb4cfd Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Thu, 24 Oct 2019 13:03:56 -0400 Subject: [PATCH 4/4] Shorter warning. --- packages/workbox-build/src/lib/errors.js | 9 ++------- packages/workbox-precaching/src/PrecacheController.ts | 10 ++-------- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/packages/workbox-build/src/lib/errors.js b/packages/workbox-build/src/lib/errors.js index 129e269b2..52ec8a837 100644 --- a/packages/workbox-build/src/lib/errors.js +++ b/packages/workbox-build/src/lib/errors.js @@ -106,13 +106,8 @@ module.exports = { manifestTransform should be an object with 'manifest' and optionally 'warnings' properties.`, 'string-entry-warning': ol`Some items were passed to additionalManifestEntries - without revisioning info. Please ensure these URLs contain versioning info - (e.g. 'https://example.com/v1.0/index.js'), as Workbox will not update them - after they are initially precached. To disable this warning, pass in an - object with a 'url' property, and a 'revision' property set to null - (e.g. {url: 'https://example.com/v1.0/index.js', revision: null}). In a - future release, using revision: null will be required. Learn - more at https://bit.ly/wb-precache`, + without revisioning info. This is generally NOT safe. Learn more at + https://bit.ly/wb-precache.`, 'no-manifest-entries-or-runtime-caching': ol`Couldn't find configuration for either precaching or runtime caching. Please ensure that the various glob options are set to match one or more files, and/or configure the diff --git a/packages/workbox-precaching/src/PrecacheController.ts b/packages/workbox-precaching/src/PrecacheController.ts index ec627342a..596d2f2bf 100644 --- a/packages/workbox-precaching/src/PrecacheController.ts +++ b/packages/workbox-precaching/src/PrecacheController.ts @@ -102,14 +102,8 @@ class PrecacheController { if (urlsToWarnAbout.length > 0) { const warningMessage = `Workbox is precaching URLs without revision ` + - `info: ${urlsToWarnAbout.join(', ')}\nOnce precached, they ` + - `will never be updated.\nThis is only safe for URLs which ` + - `contain versioning info (e.g. /v1.0/app.js or /app.1234.js, not ` + - `/index.html).\nIf your URLs do contain revision info, pass in ` + - `a object with a 'url' property and a 'revision' property set to ` + - `null (e.g. {url: '/v1.0/index.js', revision: null}).\nIn a future ` + - `release, using revision: null will be required.\nLearn more at ` + - `https://bit.ly/wb-precache`; + `info: ${urlsToWarnAbout.join(', ')}\nThis is generally NOT safe. ` + + `Learn more at https://bit.ly/wb-precache`; if (process.env.NODE_ENV === 'production') { // Use console directly to display this warning without bloating // bundle sizes by pulling in all of the logger codebase in prod.