-
Notifications
You must be signed in to change notification settings - Fork 100
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
Offline commenting #92
Conversation
); | ||
|
||
if ( ! is_admin() ) { | ||
$offline_template_url = add_query_arg( 'wp_error_template', 'offline', home_url( '/' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@todo These originally have filtering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably there should be one method for getting the offline_template_url
instead of duplicating from the error response handling logic.
wp-includes/theme-compat/error.php
Outdated
@@ -16,6 +16,7 @@ | |||
<main> | |||
<h1><?php esc_html_e( 'Oops! It looks like you’re offline.', 'pwa' ); ?></h1> | |||
<p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p> | |||
<p><!-- WP_OFFLINE_COMMENT --></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest this instead be <!--WP_OFFLINE_ERROR_MESSAGE-->
and that the line before be removed:
- <p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p>
wp-includes/theme-compat/error.php
Outdated
@@ -16,6 +16,7 @@ | |||
<main> | |||
<h1><?php esc_html_e( 'Oops! It looks like you’re offline.', 'pwa' ); ?></h1> | |||
<p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p> | |||
<p><!-- WP_OFFLINE_COMMENT --></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest this instead be <!--WP_OFFLINE_ERROR_MESSAGE-->
and that the line before be removed:
- <p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p>
headers: response.headers | ||
}; | ||
|
||
const body = text.replace( /<!-- WP_OFFLINE_COMMENT -->/, WP_OFFLINE_MESSAGE ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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( '<!--WP_OFFLINE_ERROR_MESSAGE-->', offlineMessages.comment );
And then inside of service-worker-navigation-routing.js
it would do:
const body = text.replace( '<!--WP_OFFLINE_ERROR_MESSAGE-->', offlineMessages.default );
For the general case.
wp-includes/service-workers.php
Outdated
* @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' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' ),
);
wp-includes/theme-compat/offline.php
Outdated
@@ -16,6 +16,7 @@ | |||
<main> | |||
<h1><?php esc_html_e( 'Oops! It looks like you’re offline.', 'pwa' ); ?></h1> | |||
<p><?php esc_html_e( 'Please check your internet connection, and try again.', 'pwa' ); ?></p> | |||
<p><!-- WP_OFFLINE_COMMENT --></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <!--WP_SERVICE_WORKER_ERROR_MESSAGE-->
which is used in all templates regardless.
Dependent PR has been merged, so |
Add method for template tag. Update default offline page logic.
@@ -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() ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it a method of the navigation routing component would probably be best.
@@ -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 */ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapped all this into curly brackets, are there any concerns with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea.
}; | ||
|
||
const sendOfflineResponse = () => { | ||
if ( canStreamResponse() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here starts the actual main change for the file, most of the other changes are related to wrapping it into curly brackets.
return caches.match( ERROR_OFFLINE_BODY_FRAGMENT_URL ); | ||
} | ||
|
||
return caches.match( ERROR_OFFLINE_URL ).then( function( response ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
* | ||
* @return array Array of error messages: default, comment. | ||
*/ | ||
function wp_service_worker_get_error_messages() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above too, I'm wondering if this method is necessary as a global method.
} ); | ||
|
||
// Add request to queue. | ||
queue.addRequest( req ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per GoogleChrome/workbox#1710 addRequest
is going to be replaced in v4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could update to v4 now if that is helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to update it anyway then probably would be good to do it now.
wp-includes/service-workers.php
Outdated
* 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>' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the wp_kses_post()
is necessary here. Also, should the paragraphs be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, for now let's just do:
echo '<p><!--WP_SERVICE_WORKER_ERROR_MESSAGE--></p>' ;
(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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const clone = event.request.clone(); | ||
return fetch( event.request ) | ||
.then( ( response ) => { | ||
return response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also need to account for when it is a 500 response as with the navigation handler, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
} | ||
|
||
// @todo Replace depending on BroadcastChannel. | ||
const channel = new BroadcastChannel( 'wordpress-server-errors' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
$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() ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
} | ||
); | ||
|
||
// @todo That's not actually working yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} ) | ||
.catch( () => { | ||
const bodyPromise = clone.blob(); | ||
bodyPromise.then( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this I believe should be:
return bodyPromise.then(
@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: |
@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 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 ( Thoughts? |
Ok, so when you register a route with a string |
}; | ||
|
||
wp.serviceWorker.routing.registerRoute( | ||
'/wp-comments-post.php', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to make sure this works on subdirectory installs, where /wp-comments-post.php
does not exist at the root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we can replace this with regex instead.
According to testing then no, it does not. |
const req = new Request( request.url, { | ||
method: request.method, | ||
headers: request.headers, | ||
mode: 'same-origin', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
body: body | ||
} ); | ||
|
||
// Add request to queue. @todo Replace when upgrading to Workbox v4! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This todo can now be todone due to #98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we'll have to wait until the related PR gets merged, too.
const clonedRequest = event.request.clone(); | ||
return fetch( event.request ) | ||
.then( ( response ) => { | ||
return response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
Merged the Offline Commenting Component to Navigation Routing Component within 544a747. |
Fixes #87.
Adds default Offline Commenting support:
wpPendingComments
;Temporarily adds Offline Commenting support for AMP Live List Commenting:
amp-wpPendingComments
;Related issue to be implemented later: #101