Expiration plugin throw error when cachedResponseWillBeUsed gets undefined as cachedResponse

[NoamPaz]

Library Affected:
Expiration plugin in workbox 3.0.0-alpha.3

Browser & Platform:
Version 63.0.3239.108 (Official Build) (64-bit)

Issue or Feature Request Description:
I am using expiration plugin as following:

workbox.routing.registerRoute(
    ({url}) => {
        if (url.origin === location.origin) {
            return false;
        }
        return (/^(http(s)?:)?\/\/(?!(api\.|int\.pa\.)).*(t2k|timetoknow)+.*/).test(url.href);
    },
    workbox.strategies.cacheFirst({
        cacheName: `${prefix}-${runtime}-${suffix}`,
        plugins: [
            new workbox.cacheableResponse.Plugin({
                statuses: [0, 200],
            }),
            new workbox.expiration.Plugin({
                maxAgeSeconds: 60 * 60 * 24
            })
        ]
    })
);

For the first time the cachedResponseWillBeUsed gets undefined in the cachedResponse parameter and call to _isResponseDateFresh function with the undefined response.
In this function it tries to get a date property in the header of the response which is undefined.
Here is the console error I get:

Plugin.mjs:168 Uncaught (in promise) TypeError: Cannot read property 'headers' of undefined
at Plugin._getDateHeaderTimestamp (Plugin.mjs:168)
at Plugin._isResponseDateFresh (Plugin.mjs:146)
at Plugin.cachedResponseWillBeUsed (Plugin.mjs:121)
at Object. (cacheWrapper.mjs:99)
at Generator.next ()
at step (workbox-core.dev.js:14)
at workbox-core.dev.js:25
at

[DavidScales]

I'm having a similar issue in Workbox 3.0.0-alpha.4

I was trying

workbox.routing.registerRoute(
  /(.*)articles(.*)\.(?:png|gif|jpg)/,
  workbox.strategies.cacheFirst({
    cacheName: 'images-cache',
    plugins: [
      new workbox.expiration.Plugin({
        maxEntries: 50,
        maxAgeSeconds: 30 * 24 * 60 * 60, // 30 Days
      }),
      new workbox.cacheableResponse.Plugin({
        statuses: [0, 200],
      }),
    ]
  })
);

and getting the same error:

Uncaught (in promise) TypeError: Cannot read property 'headers' of undefined
    at Plugin._getDateHeaderTimestamp (Plugin.mjs:168)
    at Plugin._isResponseDateFresh (Plugin.mjs:146)
    at Plugin.cachedResponseWillBeUsed (Plugin.mjs:121)
    at Object.<anonymous> (cacheWrapper.mjs:99)
    at Generator.next (<anonymous>)
    at step (workbox-core.dev.js:14)
    at workbox-core.dev.js:25
    at <anonymous>
	_getDateHeaderTimestamp @ Plugin.mjs:168
	_isResponseDateFresh @ Plugin.mjs:146
	cachedResponseWillBeUsed @ Plugin.mjs:121
	(anonymous) @ cacheWrapper.mjs:99
	step	@ workbox-core.dev.js:14
	(anonymous) @ workbox-core.dev.js:25
	Promise rejected (async)		
	workbox.routing.self.addEventListener.event @ default.mjs:183

I first tried a generic operation to check that I wasn't just doing something weird:

const testHandler = ({url, event}) => {
  console.log(event);
};

workbox.routing.registerRoute(
  /(.*)articles(.*)\.(?:png|gif|jpg)/,
  testHandler
);

which worked as expected. So then I removed the maxAgeSeconds property to eliminate the header check, but got a new error:

Uncaught (in promise) TypeError: Failed to execute 'put' on 'Cache': parameter 2 is not of type 'Response'.
    at Object.<anonymous> (cacheWrapper.mjs:61)
    at Generator.next (<anonymous>)
    at step (workbox-core.dev.js:14)
    at workbox-core.dev.js:25
    at <anonymous>
	(anonymous) @ cacheWrapper.mjs:61
	step	@ workbox-core.dev.js:14
	(anonymous) @ workbox-core.dev.js:25
	Promise rejected (async)		
	(anonymous) @ CacheFirst.mjs:142
	step	@ workbox-core.dev.js:14
	(anonymous) @ workbox-core.dev.js:25
	Promise resolved (async)		
	step	@ workbox-core.dev.js:24
	(anonymous) @ workbox-core.dev.js:32
	(anonymous) @ workbox-core.dev.js:11
	_getFromNetwork @ CacheFirst.mjs:151
	(anonymous) @ CacheFirst.mjs:87
	step	@ workbox-core.dev.js:14
	(anonymous) @ workbox-core.dev.js:25
	Promise resolved (async)		
	step	@ workbox-core.dev.js:24
	(anonymous) @ workbox-core.dev.js:32
	(anonymous) @ workbox-core.dev.js:11
	handle @	CacheFirst.mjs:122
	handleRequest @ Router.mjs:150
	workbox.routing.self.addEventListener.event @ default.mjs:181

which seems like it might be related. I'll dive into it a little more once I get to the office.

[gauntface]

Thanks for raising the issue - will take a look

[gauntface]

The cache expiration is a bug in Workbox. The request lifecycle can return null which the plugin was accounting for.

PR has been raised to fix this.

@DavidScales I'm not able to reproduce your error. Is there any chance you could send me an example site and / or a glitch.me example?

[DavidScales]

@gauntface

Looks like glitch.me & I aren't getting along too well but I made a quickly stripped down version of the app.

If you visit the hosted app, the service worker is installed and working. Then click on the link to "Article 1" (the erroneous route is for articles) and you should see the error in the console.

The code is here for reference.

[DavidScales]

potentially a silly question / not trying to sound impatient ;) - does this patch end up in 3.0.0-alpha.5 or is there another way for me to test it?

[gauntface]

@DavidScales you could run gulp build and host the file on your site and then in your service worker:

importScripts(`<CDN URL>`);
importScripts(`/workbox-cache-expiration.dev.js`);

<rest of code here>

That should result in the build being used instead of the CDN.

[gauntface]

@DavidScales the .5 release is now out with the fix. Would love to know if it fixed your problem.

[DavidScales]

@gauntface thanks for the advice on the gulp build thing

Currently looks like alpha.5 doesn't fix my error :(

Commenting out the cacheableResponse plugin seems to work:

workbox.routing.registerRoute(
  /(.*)articles(.*)\.(?:png|gif|jpg)/,
  workbox.strategies.cacheFirst({
    cacheName: 'images-cache',
    plugins: [
      new workbox.expiration.Plugin({
        maxEntries: 50,
        maxAgeSeconds: 30 * 24 * 60 * 60, // fixed in alpha.5
      }),
      /* still causing issues in alpha.5 */
      // new workbox.cacheableResponse.Plugin({
      //   statuses: [0, 200],
      // }),
    ]
  })
);

Also looks like it's only a problem when there is no image in the cache - if I comment out the cacheableResponse plugin, let the image cache, then uncomment the plugin, it behaves as intended.

This code is a bit above my level, but using the debugger it looks like the _isResponseSafeToCache function is converting the response that cache.put wants to put from a Response to true, which is why cache.put won't put it. If that combination of words makes sense...

[gauntface]

Moved discussion to: #1178