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

Work with a (or several) popular SW-enabled PWA plugin or theme, fork it, add our own code #8

Closed
postphotos opened this issue Jun 12, 2018 · 23 comments
Assignees
Milestone

Comments

@postphotos
Copy link

postphotos commented Jun 12, 2018

As a developer, I should have a common approach to service workers to develop a collaborative approach for PWA features.

AC1: Identify a list of WordPress plugins (from #2) of a good fit for this project.
AC2: Use our prototype approach (#7) in place of their service worker code. Determine what level of effort it took, perhaps providing a pull request back to the master plugin branch if the plugin is managed in a open repo.

@miina miina self-assigned this Jun 25, 2018
@miina
Copy link
Contributor

miina commented Jun 25, 2018

Perhaps these plugins would be a good match for looking into registering using our API prototype:

Thoughts?
cc @westonruter @kienstra @postphotos

@kienstra
Copy link
Contributor

kienstra commented Jun 26, 2018

Possible Plugins

Hi @miina,

@miina
Copy link
Contributor

miina commented Jun 27, 2018

@kienstra Great, thank you for the reviewing the plugins.

Starting work on the first plugin (https://github.com/SuperPWA/Super-Progressive-Web-Apps).

Forked repo: https://github.com/xwp/Super-Progressive-Web-Apps

@miina
Copy link
Contributor

miina commented Jun 27, 2018

Starting work on the second plugin (https://github.com/SayHelloGmbH/progressive-wordpress), work on the first plugin is on hold due to potential shortcomings of the API prototype, see the detailed comment.

Forked repo for the second plugin: https://github.com/xwp/progressive-wordpress

@miina
Copy link
Contributor

miina commented Jun 27, 2018

PR for using the API prototype within Progressive WordPress plugin:
xwp/progressive-wordpress#1

@miina
Copy link
Contributor

miina commented Jun 29, 2018

With the latest updates to SW API Prototype allowing registering dynamic scripts, starting work on the first plugin again.

@postphotos
Copy link
Author

FYI here: https://github.com/appticles/wordpress-mobile-pack made a commit just yesterday.

I think all three plugins would be great to play with for this purpose, @miina and @kienstra. Let me know if you need a hand in managing issues on those other XWP-forked repos!

@miina
Copy link
Contributor

miina commented Jun 29, 2018

PR for leveraging the API Prototype within Super Progressive Web Apps:
xwp/Super-Progressive-Web-Apps#1

@miina
Copy link
Contributor

miina commented Jun 29, 2018

Starting work on WordPress Mobile Pack plugin: see the forked repo.

@anghelalexandra
Copy link

@miina Hey, as a contributor for WordPress Mobile Pack, I can confirm that the plugin will continue to be maintained. Our team has also published other PWA plugins where we use SW (https://wordpress.org/plugins/progressive-web-apps/, https://wordpress.org/plugins/pwacommerce/), although these are not as popular as WordPress Mobile Pack.

We fully support this initiative by XWP. Right now, we instruct users to manually copy their service workers files to their domain root, which is very prone to error and we get a lot of support tickets because of this.

Another issue that we have with SW is our integration with other plugins that do Web Push Notifications. We are using OneSignal and they have their own SW. We have to modify their SW file, so we can register our SW together with theirs. It's a mess.

So, if there's anything that we can do to help, please let us know. After reading the Github issues from this repository, it's not clear to me what is your next step, but we want to be among the early adopters for this initiative.

@miina
Copy link
Contributor

miina commented Jul 5, 2018

Hey @anghelalexandra, that's great to hear, thanks for reaching out.
On manually instructing to copy the service worker files: just created a basic PR which would register the sw.js within your plugin's root directory with the Service Workers API Prototype:
xwp/wordpress-mobile-pack#1

It's not clear to me at this moment if this is the final sw.js file logic or if the sw.js file is dynamically modified as well -- haven't had the chance to look into this in more detail.

On OneSignal: which additional modifications have you had to do for making OneSignal work with your SW? Perhaps with the API just registering both scripts could work? The scripts would be concatenated in the order of registration.

On next steps: what we're trying to do at this moment is to use the API with existing plugins using service workers (like WordPress Mobile Pack). One thought on helping -- it would be great for example if you could use the created PR or create a new branch of your plugin to try using the SW API proposed by this plugin and see if all the use cases of your plugin can be accounted for by registering the Service Worker with using the API. Thoughts?

@anghelalexandra
Copy link

anghelalexandra commented Jul 5, 2018

Hey @miina,

Awesome, I'll take a look at the fork. I'm thinking to just go ahead and create a branch on our plugin that includes these PR changes and test it out.

At the moment, sw.js is not dynamically modified because we just used it for the manifest. However, we do have cases where we want to modify it - for example when caching assets for offline mode support, basically the url of the cached files should be modified depending on home_url.

Here are our instructions for integrating with OneSignal:

_Inside the OneSignal plugin directory you will find a directory called 'sdk_files' which contains 3 files. Open the 'OneSignalSDKUpdaterWorker.js' and 'OneSignalSDKWorker.js' files.

In each of the files add the following line:
echo "importScripts('https://your-domain.com/service-worker.js');";

replacing "your-domain" with your actual domain, exactly above:
`echo "importScripts('https://cdn.onesignal.com/sdks/OneSignalSDK.js');";_

It's an ideal use case for an API registering both scripts, because the issue that we are having right now is that registering service workers separately doesn't work (each plugin uses its own head script).

I actually know someone from the OneSignal team, so I'll contact them and let them know about this thread. I think they can also benefit from a unified API and their use case might be more complicated than ours.

@miina
Copy link
Contributor

miina commented Jul 5, 2018

@anghelalexandra Awesome, looking forward to hearing how it goes!

On OneSignal -- sounds like a good use case for the API indeed.

@nico-martin
Copy link
Contributor

I forked the repo and added the changes from #14 : https://github.com/nico-martin/pwa-wp
I created a new branch for my plugin: https://github.com/SayHelloGmbH/progressive-wordpress/tree/pwawp
Then I tried it all toghether and it works like a charm:
https://basic.sayhello.works/
https://basic.sayhello.works/?wp_service_worker=/
https://basic.sayhello.works/wp-json/app/v1/web-manifest
Great work from all of you 💪!

@nico-martin
Copy link
Contributor

Little update: I refactored large parts of my plugin. It now works seamless with and without xwp/pwa-wp. If the APIs from pwa-wp are aviable, it uses them, if not it uses the built in functionalities:
https://github.com/SayHelloGmbH/progressive-wordpress/tree/master
(@westonruter as you suggested, that works better than bundle both into one plugin)

@miina
Copy link
Contributor

miina commented Aug 3, 2018

Hey @anghelalexandra, did you get a chance to test the SW API Prototype out with your plugin? Any feedback so far / anything we could help with?

@anghelalexandra
Copy link

Hey @miina,

Sorry for the delay. I'm having some issues with registering the service worker. Here's what I did:

  • Installed the PWA-WP plugin
  • Installed WP Mobile Pack with the changes done from your fork

In the wp-admin panel, I'm getting this warning after activating the PWA-WP plugin:

<b>Notice</b>: has_cap was called with an argument that is <strong>deprecated</strong> since version 2.0.0! Usage of user levels is deprecated. Use capabilities instead. in <b>[...]/wp-includes/functions.php</b> on line <b>4045</b><br />

The service worker is not registered, function_exists( 'wp_register_service_worker' ) is false.

I'm running the latest WordPress version (4.9.8) on PHP 5.6, with PWA-WP 1.4.13.

@miina
Copy link
Contributor

miina commented Aug 6, 2018

Thanks for the response, @anghelalexandra!

Looks like the plugin you installed is this: https://wordpress.org/plugins/wp-pwa/ -- that's a different plugin from ours.

The correct plugin is this one: https://wordpress.org/plugins/pwa/, version 0.1, let me know if it works as expected now.

@anghelalexandra
Copy link

anghelalexandra commented Aug 6, 2018

@miina Sorry about that, I installed the correct plugin now. The wp_register_service_worker function is being called and the path to the sw file is correct, but I don't see the service worker being registered. I'm looking into Chrome developer tools console > Application > Service Workers section.

Is there anything else that I need to configure?

Btw - I'm not working on a root domain. My WordPress installation is at http://localhost/wordpress, maybe that is the problem.

@miina
Copy link
Contributor

miina commented Aug 6, 2018

@anghelalexandra Thanks for the additional information.

Since service workers require https in production then at this moment the plugin also adds https by default to the service worker URL (see here), having the site on http instead should cause the following error:
screen shot 2018-08-06 at 9 17 15 pm
Perhaps there should be an exception for localhost + debug mode since according to this link the service workers works through localhost and doesn't require https then.

Could this be the case, are you seeing any errors in the Console?

Other potential issue could be that in the http://localhost setup the $on_front_domain and $on_admin_domain both return false in these lines, would it be possible to check that as well?

@westonruter
Copy link
Collaborator

Yes, we should allow service workers on localhost to be installed over HTTP, as that is allowed by the spec. See #49.

@anghelalexandra
Copy link

@miina I'm not seeing any errors in the console. I'm running on AMPPS, so I'm on port 80.

Not sure when it should call wp_print_service_workers, I added a die() statement in there and nothing happens.

@postphotos
Copy link
Author

Closed for now, until further needs arise on this part of the project.

@westonruter westonruter added this to the 0.1 milestone Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants