Throw a WorkboxError when cacheExpiration is used without cacheName #1079

jeffposnick · 2017-11-29T20:25:46Z

I don't know what the criteria is for doing this sort of thing in NODE_ENV !== 'production' vs. unconditionally, but I went with unconditionally.

coveralls · 2017-11-29T20:35:38Z

Coverage increased (+0.03%) to 83.533% when pulling 2202357 on no-cache-expiration-on-default-cache into 7a74459 on v3.

gauntface

One thing that we may still face is that if the developer uses the plugin outside of this builder, the error will not throw.

registerRoute(new RegExp(...), strategies.cacheFirst({
  plugins: [
    new expiration.CacheExpirationPlugin()
  ]
}))

Should we add a check to: https://github.com/GoogleChrome/workbox/blob/v3/packages/workbox-cache-expiration/CacheExpirationPlugin.mjs#L114 before treating the issue as fixed?

addyosmani · 2017-11-30T02:50:49Z

Should we add a check to: https://github.com/GoogleChrome/workbox/blob/v3/packages/workbox-cache-expiration/CacheExpirationPlugin.mjs#L114 before treating the issue as fixed?

I think adding the check there before we call this fully good would be useful.

addyosmani

LGTM

jeffposnick · 2017-11-30T19:25:43Z

I'm going to revisit this PR once #1084 lands.

addyosmani · 2017-12-02T00:59:32Z

Feel free to re-explore now that #1084 has landed

jeffposnick · 2017-12-04T17:19:02Z

One thing that we may still face is that if the developer uses the plugin outside of this builder, the error will not throw.

I do not believe that the Plugin constructor would have the necessarily state information to detect whether or not there's a cache name configured. The Plugin only finds out about the associated cache name once the runtime caching takes place:

workbox/packages/workbox-cache-expiration/Plugin.mjs

Line 88 in fb016ed

_getCacheExpiration(cacheName) {

My inclination is that if we were to detect this sort of thing at runtime, it should be a warning rather than a fatal error.

But my preference would be to keep the original approach of throwing a fatal error when the strategy is misconfigured ahead of time vs. a warning at runtime.

How do other folks feel about those options?

gauntface · 2017-12-04T18:49:19Z

For dev we could a check of: 'If no cache name and a plugin of type "expiration.Plugin", throw an error' and for prod, throw at runtime.

…default-cache

coveralls · 2017-12-04T20:50:05Z

Coverage increased (+0.05%) to 84.581% when pulling ad7c879 on no-cache-expiration-on-default-cache into 526b1a2 on v3.

jeffposnick · 2017-12-06T18:35:42Z

I know that this was already approved, but I've made changes along the lines of #1079 (comment) and it would be great to have it looked over again prior to merging.

gauntface · 2017-12-08T19:20:23Z

packages/workbox-strategies/_default.mjs

@@ -56,6 +58,15 @@ const mapping = {
 const defaultExport = {};
 Object.keys(mapping).forEach((keyName) => {
  defaultExport[keyName] = (options = {}) => {
+    if (process.env.NODE_ENV !== 'production') {
+      if (options.cacheExpiration && !options.cacheName) {


This isn't needed anymore as the cacheExpiration option doesn't get used - developers have to explicitly create the Plugin.

gauntface · 2017-12-08T19:20:58Z

test/workbox-strategies/node/test-default.mjs

+    devOnly.it(`should throw when cacheExpiration is used without cacheName`, async function() {
+      await expectError(
+        () => strategies.cacheFirst({
+          cacheExpiration: {


Like wise - this test should be failing since this option does nothing now.

gauntface · 2017-12-08T19:22:34Z

packages/workbox-cache-expiration/Plugin.mjs

@@ -86,6 +88,13 @@ class Plugin {
   * @private
   */
  _getCacheExpiration(cacheName) {
+    if (cacheName === cacheNames.getRuntimeName()) {
+      throw new WorkboxError('cache-expiration-requires-cache-name', {


NIT: Maybe this error code should be 'expire-custom-caches-only'

gauntface · 2017-12-08T19:23:47Z

packages/workbox-core/models/messages/messages.mjs

+      throw new Error(`Unexpected input to 'cache-expiration-requires-cache` +
+        `-name' error.`);
+    }
+    return `You must also configure 'cacheName' when using 'cacheExpiration' ` +


This message should be changed to something like You must provide a 'cacheName' property when using the expiration Plugin with a strategy..

gauntface

I think this branch needs updating with the latest strategy code since the cacheExpiration option isn't used anymore.

workbox-pr-bot · 2017-12-11T21:20:37Z

PR-Bot Size Plugin

Changed File Sizes

File	Before	After	Change	GZipped
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js	3.13 KB	3.25 KB	+4%	1.20 KB

New Files

No new files have been added.

All File Sizes

View Table

File	Before	After	Change	GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js	3.17 KB	3.17 KB	0%	1.34 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js	1.05 KB	1.05 KB	0%	565 B
packages/workbox-build/build/_types.js	41 B	41 B	0%	61 B
packages/workbox-build/build/index.js	2.52 KB	2.52 KB	0%	1.06 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js	3.13 KB	3.25 KB	+4%	1.20 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js	593 B	593 B	0%	343 B
packages/workbox-cli/build/app.js	4.57 KB	4.57 KB	0%	1.65 KB
packages/workbox-cli/build/bin.js	2.53 KB	2.53 KB	0%	1.07 KB
packages/workbox-core/build/workbox-core.prod.js	6.67 KB	6.67 KB	0%	2.61 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js	1.94 KB	1.94 KB	0%	985 B
packages/workbox-precaching/build/workbox-precaching.prod.js	4.86 KB	4.86 KB	0%	1.89 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js	1.66 KB	1.66 KB	0%	816 B
packages/workbox-routing/build/workbox-routing.prod.js	2.74 KB	2.74 KB	0%	1.23 KB
packages/workbox-strategies/build/workbox-strategies.prod.js	3.22 KB	3.22 KB	0%	1.00 KB
packages/workbox-sw/build/workbox-sw.js	1.46 KB	1.46 KB	0%	792 B
packages/workbox-webpack-plugin/build/index.js	7.29 KB	7.29 KB	0%	2.45 KB

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

We are using 152% of our max size budget.

Total Size: 22.25KB
Percentage of Size Used: 152%

Gzipped: 8.85KB

coveralls · 2017-12-11T21:20:45Z

Coverage increased (+0.02%) to 84.77% when pulling fbddf38 on no-cache-expiration-on-default-cache into 1e980d6 on v3.

jeffposnick · 2017-12-12T15:04:59Z

Unless I've missed something else obvious, this should have the required changes now, @gauntface. Thanks for your patience.

jeffposnick added 2 commits November 29, 2017 15:23

Throw a WorkboxError when cacheExpiration is used without cacheName.

6ef0f9a

Include the message.

2202357

jeffposnick requested review from addyosmani and gauntface November 29, 2017 20:25

gauntface approved these changes Nov 29, 2017

View reviewed changes

addyosmani approved these changes Nov 30, 2017

View reviewed changes

jeffposnick added the Chillin' Not being actively worked on, or deferred for a point in the future. label Dec 1, 2017

jeffposnick added 2 commits December 4, 2017 15:23

Merge remote-tracking branch 'origin/v3' into no-cache-expiration-on-…

c784ffa

…default-cache

Review feedback.

ad7c879

jeffposnick removed the Chillin' Not being actively worked on, or deferred for a point in the future. label Dec 4, 2017

gauntface reviewed Dec 8, 2017

View reviewed changes

gauntface suggested changes Dec 8, 2017

View reviewed changes

jeffposnick added 2 commits December 11, 2017 16:09

Rebase, and updates.

ce89cb4

Linting.

fbddf38

gauntface approved these changes Dec 13, 2017

View reviewed changes

gauntface merged commit b843094 into v3 Dec 13, 2017

gauntface deleted the no-cache-expiration-on-default-cache branch December 13, 2017 01:22

GulajavaMinistudio mentioned this pull request Dec 13, 2017

Update upstream javascript-indonesias/workbox#10

Merged

jeffposnick mentioned this pull request Apr 23, 2018

Validate that options.cacheName is set when options.expiration is used #1443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw a WorkboxError when cacheExpiration is used without cacheName #1079

Throw a WorkboxError when cacheExpiration is used without cacheName #1079

jeffposnick commented Nov 29, 2017

coveralls commented Nov 29, 2017

gauntface left a comment

addyosmani commented Nov 30, 2017

addyosmani left a comment

jeffposnick commented Nov 30, 2017

addyosmani commented Dec 2, 2017

jeffposnick commented Dec 4, 2017

gauntface commented Dec 4, 2017

coveralls commented Dec 4, 2017

jeffposnick commented Dec 6, 2017

gauntface Dec 8, 2017

gauntface Dec 8, 2017

gauntface Dec 8, 2017

gauntface Dec 8, 2017

gauntface left a comment

workbox-pr-bot commented Dec 11, 2017

coveralls commented Dec 11, 2017

jeffposnick commented Dec 12, 2017

Throw a WorkboxError when cacheExpiration is used without cacheName #1079

Throw a WorkboxError when cacheExpiration is used without cacheName #1079

Conversation

jeffposnick commented Nov 29, 2017

coveralls commented Nov 29, 2017

gauntface left a comment

Choose a reason for hiding this comment

addyosmani commented Nov 30, 2017

addyosmani left a comment

Choose a reason for hiding this comment

jeffposnick commented Nov 30, 2017

addyosmani commented Dec 2, 2017

jeffposnick commented Dec 4, 2017

gauntface commented Dec 4, 2017

coveralls commented Dec 4, 2017

jeffposnick commented Dec 6, 2017

gauntface Dec 8, 2017

Choose a reason for hiding this comment

gauntface Dec 8, 2017

Choose a reason for hiding this comment

gauntface Dec 8, 2017

Choose a reason for hiding this comment

gauntface Dec 8, 2017

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

workbox-pr-bot commented Dec 11, 2017

PR-Bot Size Plugin

Changed File Sizes

New Files

All File Sizes

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

coveralls commented Dec 11, 2017

jeffposnick commented Dec 12, 2017