Skip to content
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

Service workers #463

Merged
merged 11 commits into from
Mar 8, 2021
Merged

Service workers #463

merged 11 commits into from
Mar 8, 2021

Conversation

Rich-Harris
Copy link
Member

Closes #10.

One thing I realised: this gets a little tricky when paths.assets is specified, since service workers are scoped to their own parent 'directory'. The simplest solution, at least for now, is just to disable service workers if that option is specified.

Not quite sure how to test for this. I added a sample service worker to examples/sandbox though, and confirmed that it works offline. Would just need a compliant manifest.json to be a valid PWA.


const url = new URL(request.url);

if (url.protocol === 'https:' || location.hostname === 'localhost') {
Copy link
Member

Choose a reason for hiding this comment

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

I might reverse this to reduce indentation:

if (url.protocol !== 'https:' && location.hostname !== 'localhost') {
  return;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed my mind about onFetch, and I think respondWith reads better as an if/else chain (since otherwise you have to do an early return as well as respondWith, but yeah, this would have been better

@benmccann
Copy link
Member

One thing I realised: this gets a little tricky when paths.assets is specified, since service workers are scoped to their own parent 'directory'

We've had issues in Sapper with the base option specified as well

@Rich-Harris
Copy link
Member Author

We've had issues in Sapper with the base option specified as well

Can you elaborate?

@Rich-Harris
Copy link
Member Author

Good catch, removed create_serviceworker_manifest.

I had a change of heart about onFetch etc. Deep down I think I knew I would. event.respondWith is regrettable, but the cost of adding the helper functions isn't worth it.

@benmccann
Copy link
Member

Here are the Sapper issues: sveltejs/sapper#1538 and sveltejs/sapper#1441. Not sure there's too much to be aware of here except that if we want to support base it's a case worth testing

@Rich-Harris
Copy link
Member Author

Ah ok. That should work just fine, since base is prepended to the build and assets arrays. The one place it gets a little tricky is if you test it with npm start, because in that case base and assets are disregarded. But that's a larger issue than just service workers

@Rich-Harris Rich-Harris merged commit 15402b1 into master Mar 8, 2021
@Rich-Harris Rich-Harris deleted the gh-10 branch March 8, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service workers
3 participants