Fix caching with failed requests

[thmsobrmlr]

I've been investigating an issue where users can't access our site. It seems they have sporadic network failures and thus the loading of a chunk fails. Successful responses of other requests in the same cache will now be associated with the wrong request, since the the failed responses are filtered out, but the associated requests are not. For example if the loading of chunk 3 fails, the response of chunk 4 will now be associated with the request of chunk 3. In this pull request I now filter out the associated requests as well.

[thmsobrmlr]

Unfortunately I can't correctly update the fixtures, since I get this issue:

TypeError: UglifyJsPlugin is not a constructor
    at Object.<anonymous> (/Users/thomas88/Code/offline-plugin/tests/legacy/fixtures/sw-minify-auto/webpack.config.js:14:21)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at /Users/thomas88/Code/offline-plugin/tests/legacy/run.js:43:20
    at new Promise (<anonymous>)
    at /Users/thomas88/Code/offline-plugin/tests/legacy/run.js:40:16
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! offline-plugin@5.0.6 test:fixtures:fix: `node tests/legacy/run --fix`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the offline-plugin@5.0.6 test:fixtures:fix script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/thomas88/.npm/_logs/2019-01-30T11_53_15_926Z-debug.log

Any hints what might cause this?

[Bobgy]

my PR is fixing the issue. offline-plugin is depending on UglifyJSPlugin while webpack has switched to TerserPlugin instead in latest version, so offline-plugin doesn't get UglifyJsPlugin. #436

[thmsobrmlr]

Ah, great that you have already taken control of the TerserPlugin. Thanks for letting me know!

[GGAlanSmithee]

I've made a review to the above-mentioned PR, thanks for submitting @Bobgy

[GGAlanSmithee]

@Thomas88 this looks great, do you think we should consolidate this to a function and re-use it? Not too sure, since we would have to pass both responses and requests to it.. Not too pretty, but having to update this code in 7 places the next time it's needed is not nice either, opinions?

[thmsobrmlr]

Thanks for the fast review @GGAlanSmithee :) I'm not sure if I quite understand the issue. Only code I have updated is in src/misc/sw-template.js. The code in lib/misc/sw-template.js is the transpiled output (npm run build) and code in tests/.. are test fixtures (npm run test:fixtures:fix). Or did you mean something else?

[GGAlanSmithee]

@Thomas88 🤦‍♂️ nvm my comment, a bit tired :) since Promise.all preserves order, this looks fine to me. Let's wait until #436 lands to get green checks before merging.

[GGAlanSmithee]

Looks good, thanks for the contribution.

[thmsobrmlr]

Perfect nvm :) Will update once #436 is merged to master.

[GGAlanSmithee]

@Thomas88 #436 is now merged into master 👍 (closed by misstake - sorry)

[thmsobrmlr]

@GGAlanSmithee Thanks for the heads up. I've just merged the current master and there still seem to be some issues in CI. I will investigate later today.

[thmsobrmlr]

All fixtures are correctly updated now and tests are passing. Thus ready to merge from my side. Should I squash the commits to a single one @GGAlanSmithee?

[GGAlanSmithee]

Thanks for your continued work @Thomas88 - this PR is looking good now. There is no need for you to squash, since we'll do that when merging.

Want to give @NekR a chance to voice his opinion on this before merging though, just so that I don't miss anything. It does look correct to me though.

[thmsobrmlr]

@NekR Would be really great to have your feedback on this. Don't want to push, just a small reminder that this needs qualified eyes to look over.

[NekR]

Hi @Thomas88. Sorry for a delay. I'll take a look 👍

[NekR]

This seems to be mutation the original assets array which is used for other purposes too and may cause other bugs, but I'm going to merge this and I'll fix that by copying the array in addAllNormalized.

Thank you contributing this, @Thomas88. It's very much appreciated. And sorry again for delay.

[NekR]

@GGAlanSmithee thank you too for maintaining this and taking care of offline-plugin for such a long time. 💪❤️

[GGAlanSmithee]

awesome @NekR thanks @Thomas88 ! :)

[fabiomcosta]

From my understanding, this doesn't fix the user's SW corrupted cache, is that correct?
Would you have any suggestion on how we could fix that?