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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Make all-inclusive NetworkFirst route registration optional #59

Merged
merged 12 commits into from Apr 2, 2018

Conversation

Projects
None yet
2 participants
@gerardo-rodriguez
Contributor

gerardo-rodriguez commented Apr 2, 2018

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 the nuxt.config.js Workbox settings.

Currently, the generated SW file has this line:

workboxSW.router.registerRoute(new RegExp('/.*'), workboxSW.strategies.networkFirst({}), 'GET')

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:

// nuxt.config.js

modules: [
  ['@nuxtjs/pwa', {
    workbox: {
      offline: false
    }
  }]
]

As seen in the file diff for this PR, the defaults object in the workbox/index.js file would not include the '/.*' URL pattern object in the defaults._runtimeCaching array only leaving the "_nuxt" resources runtime cache. Then, immediately after, there is a check for the offline boolean flag that optionally adds the '/.*' pattern object if true. 馃槈

I have yet to contribute to this project, so feedback is greatly appreciated. Thank you in advance! 馃槃

gerardo-rodriguez added some commits Mar 30, 2018

Merge pull request #1 from cloudfour/chore/remove-default-sw-route-reg
Remove the default `'/.*'` SW route registration
Merge pull request #2 from cloudfour/chore/remove-nuxtjs-src-modules
Remove `@nuxtjs/` directory module lookup
@gerardo-rodriguez

This comment has been minimized.

Contributor

gerardo-rodriguez commented Apr 2, 2018

FYI, I had asked a question on the NUXT community forum about this. In case anyone ran into that. 馃槈

https://nuxtjs.cmty.io/nuxt-community/pwa-module/issues/c35

@pi0

This comment has been minimized.

Member

pi0 commented Apr 2, 2018

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.

  • Looking at the changes i think moduleOptions.offline || true can be simplified to true (As default) and moving if (defaults.offline) condition after options assignment something like if (options.offline)
  • offline seems little confusing about this option. As it is for route level caching. What's your idea about cacheRoutes ?
@gerardo-rodriguez

This comment has been minimized.

Contributor

gerardo-rodriguez commented Apr 2, 2018

Thanks for your interest and PR.

@pi0 Absolutely! Thanks for all the work done on this project!

I think it could be a nice enhancement to allow disabling page-level caching or modify its regex.

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. 馃檲

Looking at the changes i think moduleOptions.offline || true can be simplified to true (As default) and moving if (defaults.offline) condition after options assignment something like if (options.offline)

Oh, good call! I can make that change! 馃槈

offline seems little confusing about this option. As it is for route level caching. What's your idea about cacheRoutes ?

This was the toughest part of this change proposal! LOL #namingThingsIsHard I went with offline only because that's what the existing comment seemed to imply, but I'm definitely open to suggestions. Regarding your suggestion to use cacheRoutes, technically, this caches more than just routes (I'm thinking the API requests described above). Hmm... 馃

@pi0

This comment has been minimized.

Member

pi0 commented Apr 2, 2018

technically, this caches more than just routes (I'm thinking the API requests described above)

Sounds reasonable. We can keep it as offline option :)

@pi0 pi0 referenced this pull request Apr 2, 2018

Closed

Only caches visited routes #24

@gerardo-rodriguez

This comment has been minimized.

Contributor

gerardo-rodriguez commented Apr 2, 2018

@pi0 Feedback has been addressed! Thanks so much! 馃槃 Let me know if you see any further changes that I should make. 馃槈

@pi0

pi0 approved these changes Apr 2, 2018

LGTM

@pi0 pi0 merged commit 76de33c into nuxt-community:master Apr 2, 2018

}
],
runtimeCaching: []
}
const options = defaultsDeep({}, this.options.workbox, moduleOptions, defaults)
// Optionally cache other routes for offline
if (options.offline) {
defaults._runtimeCaching.push({

This comment has been minimized.

@gerardo-rodriguez

gerardo-rodriguez Apr 3, 2018

Contributor

@pi0 Heads up! I just noticed this today. This should be options._runtimeCaching.push() (not defaults._runtimeCaching.push()). 馃檲

This comment has been minimized.

@pi0

pi0 Apr 3, 2018

Member

Thanks for notifying me. Will fix it :)

manniL added a commit to manniL/pwa-module that referenced this pull request Apr 4, 2018

feat(worbox): add offline option for making it optional (nuxt-communi鈥
鈥y#59)

Make all-inclusive NetworkFirst route registration optional.

Fixes nuxt-community#24.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment