New cacheWriteDidFail lifecycle callback

[jeffposnick]

trafficstars

Library Affected: workbox-core

Moved from https://github.com/GoogleChrome/workbox/pull/1505#discussion_r195156368

@philipwalton writes:

Yeah, I get the intention of the change, I'm just wondering if it could be somehow better integrated into the rest of workbox.

Did you consider creating a new lifecycle callback: maybe something like cacheWriteDidFail? And in the same way that we can register a generic fetch failure handler, we could register generic cache failure handler.

Anyway, I don't want to block this feature being added, especially if we know folks are running into quota issues today. And you've obviously thought about the trade-offs, so I'm happy with whatever you think is best.

But I think we should revisit it later. I also think it's probably worth adding a catchWriteDidFail lifecycle method at some point regardless, so strategy plugin authors can handle these cases individually without potentially affecting plugins they don't own.

[jeffposnick]

I think we can get this in for v6 as part of the overall lifecycle overhaul.

[srikanthavadhanula]

Hi @jeffposnick, I am new to open source contribution, wanted to pick this issue, as it is labeled "Good First Issue" for beginners I am going through the developer page: https://developers.google.com/web/tools/workbox/ Can you please provide me a heads-up on where to start?

Thanks

[jeffposnick]

Hello @srikanthavadhanula! Thanks so much for the interest in contributing to Workbox!

The portion of the codebase that's related to this issue is https://github.com/GoogleChrome/workbox/blob/8c49eb93d9a11431fc981f0d297247ca9484b003/packages/workbox-strategies/src/StrategyHandler.ts#L361-L372

Before the throw error; statement on line 370, the code would need to do something similar to what's going on in this piece of code: https://github.com/GoogleChrome/workbox/blob/8c49eb93d9a11431fc981f0d297247ca9484b003/packages/workbox-strategies/src/StrategyHandler.ts#L374-L382

That code iterates over cacheDidUpdate callbacks, but this new code would check for callbacks named cacheWriteDidFail, and iterate over those. I think the callback would take a similar set of parameters, excluding oldResponse.

The other piece of the puzzle would be to update https://github.com/GoogleChrome/workbox/blob/8c49eb93d9a11431fc981f0d297247ca9484b003/packages/workbox-core/src/types.ts to include information about the new callback type and parameters that it takes.

...and then the other, other piece of the puzzle would be to update the test suite in https://github.com/GoogleChrome/workbox/blob/8c49eb93d9a11431fc981f0d297247ca9484b003/test/workbox-strategies/sw/test-StrategyHandler.mjs to exercise the new callback.

There's obviously a lot of different pieces there, and we're happy to walk you through any of those specific sub-steps if you get stuck along the way. Given all that, let us know your level of comfort taking this on.

[srikanthavadhanula]

Thanks @jeffposnick This gave me a good amount of understanding of what changes need to be done. I have forked the code, trying to run locally the code and test suite. changes seem straightforward, I am trying to understand how test cases were written for existing scenarios like (cacheDidUpdate)

[jeffposnick]

That's great—if you run into any specific problems that are blocking you, please let us know!

[bonsai-rishabh]

If this issue has not been resolved yet I would love to help out if possible.

[jeffposnick]

@srikanthavadhanula, is this still something you'd want to take on?

[ghost]

@jeffposnick if this hasn't been resolved, I'd love to help out!

[jeffposnick]

Thanks, @evanrittenhouse—it doesn't sound like @srikanthavadhanula is able to help at the moment, so we would appreciate your contribution!

[jeffposnick]

(There's information about getting set up to make contributions and run the build/test suite at https://github.com/GoogleChrome/workbox/blob/v6/CONTRIBUTING.md)

[ghost]

@jeffposnick I was reading the above comment chain to try to figure out what needs to happen, but can't seem to understand - do we only need to iterate over (const callback of this.iterateCallbacks('cacheDidFail'))'? Is there a cacheDidFail callback that I need to write?

Sorry - this is my first open-source contribution (saw that it's marked as a good first issue) and am a little unclear on how to start. Any guidance/help would be much appreciated. Thank you!

[jeffposnick]

Hello @evanrittenhouse—no worries. I'm starting to wonder if marking this as a "good first issue" has ended up misleading folks, given that others have also had issues implementing it.

There is currently no cacheDidFail callback, so closing out this issue would involve creating TypeScript definitions for it and then calling any registered callbacks at the appropriate place in the codebase, getting them via this.iterateCallbacks('cacheDidFail'). And then adding in tests to ensure that it works.

We can also just assign someone from the core team to work on this if you're stuck—again, it's probably more involved than a lot of "good first issue"s would ideally be.

[ghost]

Ahh, I see @jeffposnick. Unfortunately it is probably a little bit beyond my scope - are there any other issues I can contribute to that you're aware of?