Skip to content
This repository has been archived by the owner on Nov 20, 2019. It is now read-only.

docs(example): add description of fallback handling #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

docs(example): add description of fallback handling #64

wants to merge 1 commit into from

Conversation

kontrollanten
Copy link

Closes #58

Copy link
Collaborator

@woutervanvliet woutervanvliet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Small suggestion, but other than that 👍

//
// offline-fallback.html should be defined in servierWorkerOption.assets
// to ensure it's always cached.
return global.caches.match('./offline-fallback.html')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this default makes a lot more sense to me. In order for it to work though, the offline-fallback.html page needs to be added to the cache first. Could you update line 15 accordingly.

I don't agree that serviceWorkerOptions.assets is, necessarily the right place to make sure the fallback page is cached. Perhaps define a constant in this file that defines the fallback page URL. For documentation purposes, something like this might work:

// Page to fallback to in case the resource we are trying to navigate to can't be found on
// either cache or network. Hint: Use `transformOptions` to add additional information to
// serviceWorkerOption
const FALLBACK_PAGE = serviceWorkerOption.fallbackPage || './offline-fallback.html';

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! But maybe we shall skip the serviceWorkerOption.fallbackPage part to keep it simplier? If we do that I think we can skip the descriptional comment above the FALLBACK_PAGE declaration. I think it's self describing if the reader follows the code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants