Offline commenting #92

miina · 2018-10-18T14:27:57Z

Fixes #87.

Adds default Offline Commenting support:

In case of submitting a comment while offline the default offline template with a message is displayed letting the user know the comment will be posted offline later;
Adds filter for being able to customize the default messages;
Implements background sync with queue wpPendingComments;

Temporarily adds Offline Commenting support for AMP Live List Commenting:

In case of being offline displays the relevant message under the commenting form;
Implements background sync with queue amp-wpPendingComments;
In case of successful sync and comment being submitted updates the live list;

Related issue to be implemented later: #101

miina · 2018-10-18T14:29:48Z

wp-includes/components/class-wp-service-worker-offline-commenting-component.php

+		);
+
+		if ( ! is_admin() ) {
+			$offline_template_url = add_query_arg( 'wp_error_template', 'offline', home_url( '/' ) );


@todo These originally have filtering

Probably there should be one method for getting the offline_template_url instead of duplicating from the error response handling logic.

miina · 2018-10-18T18:36:44Z

wp-includes/theme-compat/error.php

@@ -16,6 +16,7 @@
 <main>
 	<h1><?php esc_html_e( 'Oops! It looks like you&#8217;re offline.', 'pwa' ); ?></h1>
 	<p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p>
+	<p><!-- WP_OFFLINE_COMMENT --></p>


@todo This is a placeholder for testing -- the whole offline message part should be replaced with a template tag instead of just adding a new one for offline comment.

I suggest this instead be  and that the line before be removed:

- <?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?>

westonruter · 2018-10-18T18:56:26Z

wp-includes/theme-compat/error.php

@@ -16,6 +16,7 @@
 <main>
 	<h1><?php esc_html_e( 'Oops! It looks like you&#8217;re offline.', 'pwa' ); ?></h1>
 	<p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p>
+	<p><!-- WP_OFFLINE_COMMENT --></p>


I suggest this instead be  and that the line before be removed:

- <?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?>

westonruter · 2018-10-18T18:57:48Z

wp-includes/js/service-worker-offline-commenting.js

+							headers: response.headers
+						};
+
+						const body = text.replace( /<!-- WP_OFFLINE_COMMENT -->/, WP_OFFLINE_MESSAGE );


I think that WP_OFFLINE_MESSAGE will be an array of messages, including one for the offline scenario. So then this would look like:

const offlineMessages = OFFLINE_MESSAGES; /*...*/ const body = text.replace( '', offlineMessages.comment );

And then inside of service-worker-navigation-routing.js it would do:

const body = text.replace( '', offlineMessages.default );

For the general case.

westonruter · 2018-10-18T18:59:22Z

wp-includes/service-workers.php

+ * @todo This is a placeholder for offline messages, to be changed to be more general / allow multiple.
+ */
+function wp_service_worker_get_offline_message() {
+	return apply_filters( 'wp_service_worker_offline_messages', __( 'Your comment will be submitted once you are back online!', 'pwa' ) );


This should return an array of messages so like:

return apply_filters( 'wp_service_worker_offline_messages', 'default' => __( 'Please check your internet connection, and try again.', 'pwa' ), 'comment' => __( 'Your comment will be submitted once you are back online!', 'pwa' ), );

westonruter · 2018-10-18T19:03:12Z

wp-includes/theme-compat/offline.php

@@ -16,6 +16,7 @@
 <main>
 	<h1><?php esc_html_e( 'Oops! It looks like you&#8217;re offline.', 'pwa' ); ?></h1>
 	<p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p>
+	<p><!-- WP_OFFLINE_COMMENT --></p>


Instead of WP_OFFLINE_COMMENT or WP_OFFLINE_ERROR_MESSAGE as I have above, we could have a common string that is used for both offline and 500 error scenarios (per #81). So there could be just  which is used in all templates regardless.

westonruter · 2018-10-19T05:34:33Z

Dependent PR has been merged, so master can be merged back into this branch now.

…menting

Add method for template tag. Update default offline page logic.

miina · 2018-10-19T14:12:27Z

wp-includes/components/class-wp-service-worker-navigation-routing-component.php

@@ -424,6 +424,7 @@ public function serve( WP_Service_Worker_Scripts $scripts ) {
 			'BLACKLIST_PATTERNS'               => wp_service_worker_json_encode( $this->get_blacklist_patterns() ),
 			'SHOULD_STREAM_RESPONSE'           => wp_service_worker_json_encode( $should_stream_response ),
 			'STREAM_HEADER_FRAGMENT_QUERY_VAR' => wp_service_worker_json_encode( self::STREAM_FRAGMENT_QUERY_VAR ),
+			'ERROR_MESSAGES'                   => wp_service_worker_json_encode( wp_service_worker_get_error_messages() ),


@westonruter: Continued with the PR and addressed your comments, too.

Question about wp_service_worker_get_error_messages() -- do you think this method is necessary as a global method or we could just filter it here within Navigation Component?

Making it a method of the navigation routing component would probably be best.

miina · 2018-10-19T14:12:56Z

wp-includes/js/service-worker-navigation-routing.js

@@ -1,121 +1,140 @@
-/* global console, ERROR_OFFLINE_URL, ERROR_500_URL, SHOULD_STREAM_RESPONSE, STREAM_HEADER_FRAGMENT_URL, ERROR_500_BODY_FRAGMENT_URL, ERROR_OFFLINE_BODY_FRAGMENT_URL, STREAM_HEADER_FRAGMENT_QUERY_VAR, BLACKLIST_PATTERNS */
+/* global console, ERROR_OFFLINE_URL, ERROR_500_URL, SHOULD_STREAM_RESPONSE, STREAM_HEADER_FRAGMENT_URL, ERROR_500_BODY_FRAGMENT_URL, ERROR_OFFLINE_BODY_FRAGMENT_URL, STREAM_HEADER_FRAGMENT_QUERY_VAR, BLACKLIST_PATTERNS, ERROR_MESSAGES */
+{


Wrapped all this into curly brackets, are there any concerns with it?

That's a good idea.

miina · 2018-10-19T14:14:13Z

wp-includes/js/service-worker-navigation-routing.js

+			};
+
+			const sendOfflineResponse = () => {
+				if ( canStreamResponse() ) {


Here starts the actual main change for the file, most of the other changes are related to wrapping it into curly brackets.

miina · 2018-10-19T14:16:05Z

wp-includes/js/service-worker-navigation-routing.js

+					return caches.match( ERROR_OFFLINE_BODY_FRAGMENT_URL );
+				}
+
+				return caches.match( ERROR_OFFLINE_URL ).then( function( response ) {


This mostly duplicates the handling within service-worker-offline-commenting.js, except for the errorMessages.default vs errorMessage.comment. Wondering if we should create a global method instead?

miina · 2018-10-19T14:16:51Z

wp-includes/service-workers.php

+ *
+ * @return array Array of error messages: default, comment.
+ */
+function wp_service_worker_get_error_messages() {


As mentioned above too, I'm wondering if this method is necessary as a global method.

miina · 2018-10-19T14:21:30Z

wp-includes/js/service-worker-offline-commenting.js

+						} );
+
+						// Add request to queue.
+						queue.addRequest( req );


Per GoogleChrome/workbox#1710 addRequest is going to be replaced in v4.

We could update to v4 now if that is helpful.

If we're going to update it anyway then probably would be good to do it now.

westonruter · 2018-10-21T03:34:02Z

wp-includes/service-workers.php

+ * Display service worker error message template tag.
+ */
+function wp_service_worker_error_message_placeholder() {
+	echo wp_kses_post( '<p><!--WP_SERVICE_WORKER_ERROR_MESSAGE--></p>' );


I don't think the wp_kses_post() is necessary here. Also, should the paragraphs be included?

Not sure about the paragraphs -- I was thinking at first that maybe not but then the developer would always have to add the markup instead of just overriding the text message. Without markup it would look like this:

So basically I was thinking that maybe for the default template we should add paragraph and try to avoid "breaking" how the default offline template looks like.

However, I'm fine removing it.

OK, for now let's just do:

echo '' ;

(No need for wp_kses_post().)

I think I was thinking of a case where there is no message. But I suppose there would always be one.

westonruter · 2018-10-22T01:23:14Z

wp-includes/components/class-wp-service-worker-navigation-routing-component.php

@@ -473,8 +474,9 @@ public function get_priority() {
 	 * @return string Script.
 	 */
 	public function get_script() {
-		$script = file_get_contents( PWA_PLUGIN_DIR . '/wp-includes/js/service-worker-navigation-routing.js' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
-		$script = preg_replace( '#/\*\s*global.+?\*/#', '', $script );
+		$script  = file_get_contents( PWA_PLUGIN_DIR . '/wp-includes/js/service-worker-offline-commenting.js' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents


I was just thinking that perhaps it would make sense to remove commenting from the navigation routing component to move it into its own component. Is not the main reason why it needs to be placed here because the component has a super early priority of -9999? Shouldn't actually the navigation routing component have a super late priority? It should essentially be the default handler in case another doesn't catch it, such as the wp-comments-post.php handler you register.

Originally it was implemented separately but then moved to here since it seemed to make sense due to sharing some logic (getting the URL-s to replace for example, $offline_error_precache_entry). Makes sense though that the default handler should probably have a late priority! Will move the commenting separately.

Also getting the error message would be shared then so in case of moving it separately we could maybe still have wp_service_worker_get_error_messages as a global method.

westonruter · 2018-10-22T01:25:37Z

wp-includes/js/service-worker-offline-commenting.js

+		const clone = event.request.clone();
+		return fetch( event.request )
+			.then( ( response ) => {
+				return response;


This should also need to account for when it is a 500 response as with the navigation handler, right?

Yes. These two scripts (navigation and commenting) would have duplicate code this way, should we move the handleRequest method to be a global method instead?

How about adding a new script which is marked as a dependency for both offline commenting and navigation routing which defines these two functions for handling the response and offline response, and then we can re-use these functions in both places.

Hurray for script dependencies!

Perhaps these functions should be defined off of wp.serviceWorker. Nevertheless, beware:

https://github.com/xwp/pwa-wp/blob/c3b06fe6f7cfc78538c2b256448a41e4a050602d/wp-includes/js/service-worker.js#L134-L141

miina · 2018-10-23T18:17:16Z

wp-includes/js/service-worker-offline-commenting.js

+				}
+
+				// @todo Replace depending on BroadcastChannel.
+				const channel = new BroadcastChannel( 'wordpress-server-errors' );


I added currently the logic here without creating global methods (yet) to see what's needed exactly. Also removed the streaming from here since it didn't seem to be necessary in case of wp-comments-post.

That's right. No need to do streaming in handling wp-comments-post.php requests. It should be redirecting, or serving the offline page in its entirety.

miina · 2018-10-23T18:20:01Z

wp-includes/components/class-wp-service-worker-offline-commenting-component.php

+		$this->replacements = array(
+			'ERROR_OFFLINE_URL' => wp_service_worker_json_encode( isset( $offline_error_precache_entry['url'] ) ? $offline_error_precache_entry['url'] : null ),
+			'ERROR_500_URL'     => wp_service_worker_json_encode( isset( $server_error_precache_entry['url'] ) ? $server_error_precache_entry['url'] : null ),
+			'ERROR_MESSAGES'    => wp_service_worker_json_encode( wp_service_worker_get_error_messages() ),


@westonruter Created a separate component -- above you can see that getting these 3 URLs is just a duplication of Routing Component. Perhaps we should move the logic for getting the URLs out of the components to avoid duplication.

westonruter · 2018-10-25T20:22:45Z

amp/amp-service-worker-offline-commenting.js

+					}
+				);
+
+				// @todo That's not actually working yet.


I think you have to move this into the catch function, like so:

const bodyPromise = clone.blob(); bodyPromise.then( function( body ) { const request = event.request; const req = new Request( request.url, { method: request.method, headers: request.headers, mode: 'same-origin', credentials: request.credentials, referrer: request.referrer, redirect: 'manual', body: body } ); // Add request to queue. @todo Replace when upgrading to Workbox v4! queue.addRequest( req ); const body = JSON.stringify( { 'error': errorMessages.comment } ); return new Response( body, {} ); } );

You only want it to be sent back in the case of an error (catch) scenario. Also, you should wait to send it until the bodyPromise resolves, as otherwise I believe it the callback can be killed.

westonruter · 2018-10-25T20:54:18Z

amp/amp-service-worker-offline-commenting.js

+			} )
+			.catch( () => {
+				const bodyPromise = clone.blob();
+				bodyPromise.then(


Also this I believe should be:

return bodyPromise.then(

westonruter · 2018-10-25T20:58:20Z

@miina I just realized that it would be important to not unregister the normal comment request handler (as with #94) because on a paired mode site, comments could be submitted outside of AMP as well. So both routes need to remain registered. Perhaps then this shows that all of the custom router logic should be actually eliminated (in a separate PR) since there is a real use case for multiple routes to match:

https://github.com/xwp/pwa-wp/blob/c3b06fe6f7cfc78538c2b256448a41e4a050602d/wp-includes/js/service-worker.js#L15-L141

miina · 2018-10-25T21:05:57Z

@westonruter In case of Offline Commenting we're actually not going to need to unregister the default commenting since at least according to testing then these two registered routes are not conflicting and also not causing a warning (one is strictly matching /wp-comments-post.php and the other matching the route with specific parameters.

It still seems to me that it would be good to display a warning in case of conflicting routes since if there are for example two scripts that address exactly the same route, for example the default commenting (/wp-comments-post.php), and it's not working then as a developer it would be good to get the information that it's due to conflicts. The custom router is not doing anything else except for just displaying a warning in develop mode.

Thoughts?

westonruter · 2018-10-25T22:43:26Z

(one is strictly matching /wp-comments-post.php and the other matching the route with specific parameters.

Ok, so when you register a route with a string'/wp-comments-post.php' then Workbox will not allow query params?

westonruter · 2018-10-25T22:45:17Z

wp-includes/js/service-worker-offline-commenting.js

+	};
+
+	wp.serviceWorker.routing.registerRoute(
+		'/wp-comments-post.php',


We'll need to make sure this works on subdirectory installs, where /wp-comments-post.php does not exist at the root.

Good point, we can replace this with regex instead.

miina · 2018-10-25T22:52:36Z

Ok, so when you register a route with a string'/wp-comments-post.php' then Workbox will not allow query params?

According to testing then no, it does not.

westonruter · 2018-10-26T18:16:02Z

amp/amp-service-worker-offline-commenting.js

+						const req = new Request( request.url, {
+							method: request.method,
+							headers: request.headers,
+							mode: 'same-origin',


I think that mode will need to be set to cors since when submitting a comment from the AMP Cache it will need to connect to the origin.

…opied mode

westonruter · 2018-10-30T17:22:57Z

amp/amp-service-worker-offline-commenting.js

+						body: body
+					} );
+
+					// Add request to queue. @todo Replace when upgrading to Workbox v4!


This todo can now be todone due to #98

Looks like we'll have to wait until the related PR gets merged, too.

westonruter · 2018-10-30T17:40:18Z

amp/amp-service-worker-offline-commenting.js

+		const clonedRequest = event.request.clone();
+		return fetch( event.request )
+			.then( ( response ) => {
+				return response;


Is there any need for this then handler now? Should it not just do:

fetch( event.request ).catch( () => {

In reality a success response should always be a 302 redirect, correct? That is, unless AMP live list commenting is being used in which case it would be a 200 or 202 response.

In particular, there is the handleResponse function.

Given that the 302 response is irrelevant for streaming or app shell, we don't need any of that logic. But some 500 error handling would perhaps be nice, but not essential for now. So I think we can just eliminate the then.

miina · 2018-10-31T00:37:53Z

Merged the Offline Commenting Component to Navigation Routing Component within 544a747.

miina added 2 commits October 17, 2018 17:29

Add workbox background sync for comments.

7c74cba

Fix phpcs.

93f5c5f

miina commented Oct 18, 2018

View reviewed changes

miina added 4 commits October 18, 2018 13:34

Add workbox background sync for comments.

44b7747

Fix phpcs.

ca07c6d

Changes with rebasing the PR.

f6472eb

Resolve conflicts.

3cd1a33

miina changed the base branch from master to add/sw-streaming October 18, 2018 16:53

Add custom message for commenting.

7d67528

miina commented Oct 18, 2018

View reviewed changes

westonruter reviewed Oct 18, 2018

View reviewed changes

westonruter changed the base branch from add/sw-streaming to master October 19, 2018 05:33

miina added 2 commits October 19, 2018 10:28

Merge remote-tracking branch 'origin/master' into feature/offline-com…

61b7b7f

…menting

Update error messages.

df6b0dc

Add method for template tag. Update default offline page logic.

miina commented Oct 19, 2018

View reviewed changes

Add todo.

4e7381c

westonruter reviewed Oct 21, 2018

View reviewed changes

westonruter reviewed Oct 22, 2018

View reviewed changes

miina added 2 commits October 23, 2018 15:10

Create separate component for Offline Editing.

20bba99

Merge master & resolve conflicts.

8a62653

miina commented Oct 23, 2018

View reviewed changes

Start adding script for AMP Live List.

08250a5

westonruter mentioned this pull request Oct 24, 2018

Add navigation caching strategy #89

Merged

westonruter reviewed Oct 25, 2018

View reviewed changes

Modify sending response.

78a7dcb

westonruter reviewed Oct 25, 2018

View reviewed changes

westonruter reviewed Oct 26, 2018

View reviewed changes

miina and others added 6 commits October 26, 2018 16:27

Use regex for default offline commenting route.

4da1a3f

Flatten commentHandler

69eb395

Prevent default comment handler from also matching AMP comment handler

a3ce4b6

Add Access-Control-Allow-Origin response header

138b788

Fix promise chain (undo 69eb395); use 202 status, follow redirects, c…

c4c88b3

…opied mode

Use response with message property instead of error property

de7b2eb

westonruter mentioned this pull request Oct 27, 2018

Upgrade Workbox to v4.0.0-alpha.0 #98

Merged

Remove todo comment.

e631395

westonruter reviewed Oct 30, 2018

View reviewed changes

Merge master.

9594969

westonruter reviewed Oct 30, 2018

View reviewed changes

miina added 2 commits October 30, 2018 18:23

Remove redundant .then from fetch and only catch errors.

8c5dd63

Merge comments component back into navigation component.

544a747

miina changed the title ~~[WIP] Offline commenting~~ Offline commenting Oct 31, 2018

westonruter approved these changes Oct 31, 2018

View reviewed changes

westonruter merged commit 1efb6a9 into master Oct 31, 2018

westonruter deleted the feature/offline-commenting branch October 31, 2018 16:09

westonruter added the service-workers label Feb 25, 2019

westonruter added this to the 0.2 milestone Apr 16, 2019

Offline commenting #92

Offline commenting #92

Conversation

miina commented Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Oct 25, 2018

miina commented Oct 25, 2018

westonruter commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented Oct 31, 2018

miina commented Oct 18, 2018 •

edited

Loading

miina Oct 23, 2018 •

edited

Loading