-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Make all-inclusive NetworkFirst route registration optional #59
Feature: Make all-inclusive NetworkFirst route registration optional #59
Conversation
Remove the default `'/.*'` SW route registration
Remove `@nuxtjs/` directory module lookup
FYI, I had asked a question on the NUXT community forum about this. In case anyone ran into that. 😉 |
Hey @gerardo-rodriguez. Thanks for your interest and PR. I think it could be a nice enhancement to allow disabling page-level caching or modify its regex.
|
@pi0 Absolutely! Thanks for all the work done on this project!
For sure, one of the issues this is causing us is all application API requests are inadvertently being cached using the NetworkFirst caching strategy. If the app went offline, the cached API responses would be used. If this application was intended to be an offline application, this would be great! But, it is not intended to be an offline app and providing the user old API cached responses will actually cause more UX confusion than actually helping. 🙈
Oh, good call! I can make that change! 😉
This was the toughest part of this change proposal! LOL #namingThingsIsHard I went with |
Sounds reasonable. We can keep it as |
@pi0 Feedback has been addressed! Thanks so much! 😄 Let me know if you see any further changes that I should make. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
], | ||
runtimeCaching: [] | ||
} | ||
|
||
const options = defaultsDeep({}, this.options.workbox, moduleOptions, defaults) | ||
|
||
// Optionally cache other routes for offline | ||
if (options.offline) { | ||
defaults._runtimeCaching.push({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pi0 Heads up! I just noticed this today. This should be options._runtimeCaching.push()
(not defaults._runtimeCaching.push()
). 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for notifying me. Will fix it :)
…ty#59) Make all-inclusive NetworkFirst route registration optional. Fixes nuxt-community#24.
What problem does this PR solve?
There exists an assumption that all applications using the NUXT PWA module should, by default, use a runtime NetworkFirst caching strategy for all assets that match the
'/.*'
route. Some applications (including the current one we are working on that is using the@nuxtjs/pwa
module) make this assumption invalid. This configuration should be optional and configurable via thenuxt.config.js
Workbox settings.Currently, the generated SW file has this line:
Instead of making this a default, it should be optional to include this functionality.
Proposed changes
The
nuxt.config.js
file should allow a Boolean flag to control adding the'/.*'
NetworkFirst runtime caching route registration.Example
nuxt.config.js
configuration:As seen in the file diff for this PR, the
defaults
object in theworkbox/index.js
file would not include the'/.*'
URL pattern object in thedefaults._runtimeCaching
array only leaving the "_nuxt" resources runtime cache. Then, immediately after, there is a check for theoffline
boolean flag that optionally adds the'/.*'
pattern object iftrue
. 😉I have yet to contribute to this project, so feedback is greatly appreciated. Thank you in advance! 😄