search

NekR / offline-plugin

Offline plugin (ServiceWorker, AppCache) for webpack (https://webpack.js.org/)
MIT License
4.52k stars 295 forks source link

Fix caching with failed requests #437

Closed thmsobrmlr closed 5 years ago

thmsobrmlr commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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. https://github.com/NekR/offline-plugin/pull/436

thmsobrmlr commented 5 years ago

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

GGAlanSmithee commented 5 years ago

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

GGAlanSmithee commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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.

thmsobrmlr commented 5 years ago

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

GGAlanSmithee commented 5 years ago

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

thmsobrmlr commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

NekR commented 5 years ago

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 commented 5 years ago

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

GGAlanSmithee commented 5 years ago

awesome @NekR thanks @thomas88 ! :)

fabiomcosta commented 4 years ago

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?