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.

Sign up for GitHub

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

Jump to bottom

feat(android): proxy service worker requests through local server #452

Merged
merged 3 commits into from
Apr 14, 2020
Merged

feat(android): proxy service worker requests through local server #452

merged 3 commits into from
Apr 14, 2020

Conversation

dirkgroenen
Copy link
Contributor

Currently Service Workers are not able to retrieve files served by the local file server. This
change fixes this, allowing to use PWA-like offline caching utils.

This fixes #410

@dirkgroenen
Copy link
Contributor Author

Anyone from the Ionic team (@jcesarmobile perhaps) who wants to take a look at this one?

@JohnRSim
Copy link

JohnRSim commented Jan 8, 2020 •
edited
Loading

Any thoughts or updates on this one- @jcesarmobile?
Can we get this update in - Thanks :)

@JohnRSim
Copy link

JohnRSim commented Jan 8, 2020

also noticed the file was missing the imports -

import android.webkit.ServiceWorkerController;
import android.webkit.ServiceWorkerClient;

@dirkgroenen
feat(ionicwebviewengine): proxy service worker requests through local…
2912a16 
… server

Currently Service Workers are not able to retrieve files served by the local file server. This
change fixes this, allowing to use PWA-like offline caching utils.

fix #410
@dirkgroenen
Add missing import
ade6cec
@dirkgroenen
Copy link
Contributor Author

Thanks @JohnRSim, I've added the missing imports.

Can we please get a response from the Ionic Team (@jcesarmobile or @manucorporat perhaps) whether this PR will be merged or not? Thanks 👍

@nphyatt
Copy link
Contributor

nphyatt commented Mar 25, 2020

This looks nice but the only fear would be that current Ionic Devs don't expect service workers to work in their app and if they all of the sudden start working that may have unintended consequences for those apps.

@dirkgroenen
Copy link
Contributor Author

@nphyatt thanks for your reply, appreciate.
Which circumstances are you thinking about where Service Workers would suddenly start to work? Personally I can hardly imagine devs are currently trying to load Service Workers without caring for their returned Error, causing them to suddenly work as soon as this change is implemented.

If we do find this to be a valid concern we could always consider making it configurable or something, can't we?

@tryhardest tryhardest mentioned this pull request Mar 31, 2020
Closed
@nphyatt
Copy link
Contributor

nphyatt commented Apr 7, 2020

It never ceases to amaze me what people might be doing 😄 but yes if we had an opt-in config flag that would ease my worries 😅

@dirkgroenen
Copy link
Contributor Author

dirkgroenen commented Apr 9, 2020 •
edited
Loading

Haha, true. Would you consider merging this PR when there's an opt-in config flag? If so I'll make sure to add that 😉

@jcesarmobile
Copy link
Member

yeah, if you make it as opt-in and disabled by default we will merge

@dirkgroenen
Copy link
Contributor Author

Lovely, just pushed some changes making it configurable.

@JohnRSim
Copy link

Looks good.

@jcesarmobile jcesarmobile self-requested a review April 14, 2020 15:41
jcesarmobile
jcesarmobile approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

looks good

@jcesarmobile jcesarmobile changed the title feat(ionicwebviewengine): proxy service worker requests through local server feat(android): proxy service worker requests through local server Apr 14, 2020
@jcesarmobile jcesarmobile merged commit c672175 into ionic-team:master Apr 14, 2020
Ionitron added a commit that referenced this pull request Apr 14, 2020
@Ionitron
chore(release): 4.2.0 [skip ci]
9e8d35f 
# [4.2.0](v4.1.3...v4.2.0) (2020-04-14)

### Bug Fixes

* **ionassethandler.m:** fix startPath is getting null ([#463](#463)) ([0bf16f1](0bf16f1))
* **ios:** avoid app scrolling to top on keyboard hide ([#533](#533)) ([7974eb4](7974eb4))
* **ios:** Replace deprecated APIs ([#539](#539)) ([27b9021](27b9021))

### Features

* **android:** proxy service worker requests through local server ([#452](#452)) ([c672175](c672175))
* **ios:** implement custom userAgent handling ([#537](#537)) ([8587114](8587114))
@Ionitron
Copy link
Collaborator

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dirkgroenen dirkgroenen deleted the feature/proxy-sw-requests branch April 15, 2020 08:16
@snyk-bot snyk-bot mentioned this pull request Apr 17, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request May 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcesarmobile jcesarmobile jcesarmobile approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Service worker (workbox) with android webview error
5 participants
@dirkgroenen @JohnRSim @nphyatt @jcesarmobile @Ionitron