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

Offline commenting #92

Merged
merged 24 commits into from
Oct 31, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7c74cba
Add workbox background sync for comments.
miina Oct 17, 2018
93f5c5f
Fix phpcs.
miina Oct 17, 2018
44b7747
Add workbox background sync for comments.
miina Oct 17, 2018
ca07c6d
Fix phpcs.
miina Oct 17, 2018
f6472eb
Changes with rebasing the PR.
miina Oct 18, 2018
3cd1a33
Resolve conflicts.
miina Oct 18, 2018
7d67528
Add custom message for commenting.
miina Oct 18, 2018
61b7b7f
Merge remote-tracking branch 'origin/master' into feature/offline-com…
miina Oct 19, 2018
df6b0dc
Update error messages.
miina Oct 19, 2018
4e7381c
Add todo.
miina Oct 19, 2018
20bba99
Create separate component for Offline Editing.
miina Oct 23, 2018
8a62653
Merge master & resolve conflicts.
miina Oct 23, 2018
08250a5
Start adding script for AMP Live List.
miina Oct 24, 2018
78a7dcb
Modify sending response.
miina Oct 25, 2018
4da1a3f
Use regex for default offline commenting route.
miina Oct 26, 2018
69eb395
Flatten commentHandler
westonruter Oct 26, 2018
a3ce4b6
Prevent default comment handler from also matching AMP comment handler
westonruter Oct 26, 2018
138b788
Add Access-Control-Allow-Origin response header
westonruter Oct 26, 2018
c4c88b3
Fix promise chain (undo 69eb3951); use 202 status, follow redirects, …
westonruter Oct 27, 2018
de7b2eb
Use response with message property instead of error property
westonruter Oct 27, 2018
e631395
Remove todo comment.
miina Oct 30, 2018
9594969
Merge master.
miina Oct 30, 2018
8c5dd63
Remove redundant .then from fetch and only catch errors.
miina Oct 30, 2018
544a747
Merge comments component back into navigation component.
miina Oct 31, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions amp/amp-service-worker-offline-commenting.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/* global ERROR_MESSAGES, SITE_URL */
{
const queue = new wp.serviceWorker.backgroundSync.Queue( 'amp-wpPendingComments' );
const errorMessages = ERROR_MESSAGES;

const commentHandler = ( { event } ) => {
const clone = event.request.clone();
return fetch( event.request )
.then( ( response ) => {
// @todo Make sure that 409 etc. error still work as expected.
return response;
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

} )
.catch( () => {
const bodyPromise = clone.blob();
return bodyPromise.then(
function( body ) {
const request = event.request;
const req = new Request( request.url, {
method: request.method,
headers: request.headers,
mode: 'same-origin',
Copy link
Collaborator

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.

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 jsonBody = JSON.stringify( { 'error': errorMessages.comment } );
return new Response( jsonBody, {
headers: {
'Access-Control-Allow-Credentials': 'true',
'Content-Type': 'application/json; charset=UTF-8',
'Access-Control-Expose-Headers': 'AMP-Access-Control-Allow-Source-Origin',
'AMP-Access-Control-Allow-Source-Origin': SITE_URL,
'Cache-Control': 'no-cache, must-revalidate, max-age=0'
}
} );
}
);
} );
};

wp.serviceWorker.routing.registerRoute(
new RegExp('/wp-comments-post\.php\?.*_wp_amp_action_xhr_converted.*' ), // eslint-disable-line no-useless-escape
commentHandler,
'POST'
);
}
38 changes: 38 additions & 0 deletions amp/class-amp-service-worker.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public function init() {
add_action( 'wp', array( $this, 'add_install_hooks' ) );
add_action( 'wp_front_service_worker', array( $this, 'add_amp_runtime_caching' ) );
add_action( 'wp_front_service_worker', array( $this, 'add_image_runtime_caching' ) );
add_action( 'wp_front_service_worker', array( $this, 'add_live_list_offline_commenting' ) );
}

/**
Expand Down Expand Up @@ -124,6 +125,43 @@ public function add_image_runtime_caching( $service_workers ) {
);
}

/**
* Add live list offline commenting service worker script.
*
* @param object $service_workers WP Service Workers object.
*/
public function add_live_list_offline_commenting( $service_workers ) {
if ( ! ( $service_workers instanceof WP_Service_Worker_Scripts ) ) {
_doing_it_wrong( __METHOD__, esc_html__( 'Expected argument to be WP_Service_Worker_Scripts.', 'amp' ), '1.0' );
return;
}

$theme_support = AMP_Theme_Support::get_theme_support_args();
if ( empty( $theme_support['comments_live_list'] ) ) {
return;
}

$service_workers->register(
'amp-offline-commenting',
function() {
$js = file_get_contents( dirname( __FILE__ ) . '/amp-service-worker-offline-commenting.js' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents, WordPress.WP.AlternativeFunctions.file_system_read_file_get_contents
$js = preg_replace( '#/\*\s*global.+?\*/#', '', $js );
$js = str_replace(
'ERROR_MESSAGES',
wp_json_encode( wp_service_worker_get_error_messages() ),
$js
);
$js = str_replace(
'SITE_URL',
wp_json_encode( site_url() ),
$js
);
return $js;
}
);

}

/**
* Register URLs that will be precached in the runtime cache. (Yes, this sounds somewhat strange.)
*
Expand Down
1 change: 1 addition & 0 deletions pwa.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@

/** WP_Service_Worker_Component Implementation Classes */
require_once PWA_PLUGIN_DIR . '/wp-includes/components/class-wp-service-worker-configuration-component.php';
require_once PWA_PLUGIN_DIR . '/wp-includes/components/class-wp-service-worker-offline-commenting-component.php';
require_once PWA_PLUGIN_DIR . '/wp-includes/components/class-wp-service-worker-navigation-routing-component.php';
require_once PWA_PLUGIN_DIR . '/wp-includes/components/class-wp-service-worker-precaching-routes-component.php';
require_once PWA_PLUGIN_DIR . '/wp-includes/components/class-wp-service-worker-precaching-routes.php';
Expand Down
1 change: 1 addition & 0 deletions wp-includes/class-wp-service-workers.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public function __construct() {
'navigation_routing' => new WP_Service_Worker_Navigation_Routing_Component(),
'precaching_routes' => new WP_Service_Worker_Precaching_Routes_Component(),
'caching_routes' => new WP_Service_Worker_Caching_Routes_Component(),
'offline_commenting' => new WP_Service_Worker_Offline_Commenting_Component(),
);

$this->scripts = new WP_Service_Worker_Scripts( $components );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() ),
Copy link
Contributor Author

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?

Copy link
Collaborator

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.

);
}

Expand Down Expand Up @@ -464,7 +465,7 @@ public function get_blacklist_patterns() {
* @return int Hook priority. A higher number means a lower priority.
*/
public function get_priority() {
return -99999;
return 99;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
<?php
/**
* WP_Service_Worker_Offline_Commenting_Component class.
*
* @package PWA
*/

/**
* Class representing the service worker core component for handling offline commenting.
*
* @since 0.2
*/
class WP_Service_Worker_Offline_Commenting_Component implements WP_Service_Worker_Component {

/**
* Internal storage for replacements to make in the error response handling script.
*
* @since 0.2
* @var array
*/
protected $replacements = array();

/**
* Adds the component functionality to the service worker.
*
* @todo This is all copied from navigation routing component and duplication should be removed.
*
* @since 0.2
*
* @param WP_Service_Worker_Scripts $scripts Instance to register service worker behavior with.
*/
public function serve( WP_Service_Worker_Scripts $scripts ) {
$template = get_template();
$stylesheet = get_stylesheet();

$revision = sprintf( '%s-v%s', $template, wp_get_theme( $template )->Version );
if ( $template !== $stylesheet ) {
$revision .= sprintf( ';%s-v%s', $stylesheet, wp_get_theme( $stylesheet )->Version );
}

// Ensure the user-specific offline/500 pages are precached, and thet they update when user logs out or switches to another user.
$revision .= sprintf( ';user-%d', get_current_user_id() );

if ( ! is_admin() ) {
$offline_error_template_file = pwa_locate_template( array( 'offline.php', 'error.php' ) );
$offline_error_precache_entry = array(
'url' => add_query_arg( 'wp_error_template', 'offline', home_url( '/' ) ),
'revision' => $revision . ';' . md5( $offline_error_template_file . file_get_contents( $offline_error_template_file ) ), // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
);
$server_error_template_file = pwa_locate_template( array( '500.php', 'error.php' ) );
$server_error_precache_entry = array(
'url' => add_query_arg( 'wp_error_template', '500', home_url( '/' ) ),
'revision' => $revision . ';' . md5( $server_error_template_file . file_get_contents( $server_error_template_file ) ), // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
);

/**
* Filters what is precached to serve as the offline error response on the frontend.
*
* The URL returned in this array will be precached by the service worker and served as the response when
* the client is offline or their connection fails. To prevent this behavior, this value can be filtered
* to return false. When a theme or plugin makes a change to the response, the revision value in the array
* must be incremented to ensure the URL is re-fetched to store in the precache.
*
* @since 0.2
*
* @param array|false $entry {
* Offline error precache entry.
*
* @type string $url URL to page that shows the offline error template.
* @type string $revision Revision for the template. This defaults to the template and stylesheet names, with their respective theme versions.
* }
*/
$offline_error_precache_entry = apply_filters( 'wp_offline_error_precache_entry', $offline_error_precache_entry );

/**
* Filters what is precached to serve as the internal server error response on the frontend.
*
* The URL returned in this array will be precached by the service worker and served as the response when
* the server returns a 500 internal server error . To prevent this behavior, this value can be filtered
* to return false. When a theme or plugin makes a change to the response, the revision value in the array
* must be incremented to ensure the URL is re-fetched to store in the precache.
*
* @since 0.2
*
* @param array $entry {
* Server error precache entry.
*
* @type string $url URL to page that shows the server error template.
* @type string $revision Revision for the template. This defaults to the template and stylesheet names, with their respective theme versions.
* }
*/
$server_error_precache_entry = apply_filters( 'wp_server_error_precache_entry', $server_error_precache_entry );

} else {
$offline_error_precache_entry = array(
'url' => add_query_arg( 'code', 'offline', admin_url( 'admin-ajax.php?action=wp_error_template' ) ), // Upon core merge, this would use admin_url( 'error.php' ).
'revision' => PWA_VERSION, // Upon core merge, this should be the core version.
);
$server_error_precache_entry = array(
'url' => add_query_arg( 'code', '500', admin_url( 'admin-ajax.php?action=wp_error_template' ) ), // Upon core merge, this would use admin_url( 'error.php' ).
'revision' => PWA_VERSION, // Upon core merge, this should be the core version.
);
}

$scripts->register(
'wp-offline-commenting',
array(
'src' => array( $this, 'get_script' ),
'deps' => array( 'wp-base-config' ),
)
);

if ( $offline_error_precache_entry ) {
$scripts->precaching_routes()->register( $offline_error_precache_entry['url'], isset( $offline_error_precache_entry['revision'] ) ? $offline_error_precache_entry['revision'] : null );
}
if ( $server_error_precache_entry ) {
$scripts->precaching_routes()->register( $server_error_precache_entry['url'], isset( $server_error_precache_entry['revision'] ) ? $server_error_precache_entry['revision'] : null );
}

$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() ),
Copy link
Contributor Author

@miina miina Oct 23, 2018

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.

);
}

/**
* Gets the priority this component should be hooked into the service worker action with.
*
* @since 0.2
*
* @return int Hook priority. A higher number means a lower priority.
*/
public function get_priority() {
return 10;
}

/**
* Get script for routing navigation requests.
*
* @return string Script.
*/
public function get_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
$script = preg_replace( '#/\*\s*global.+?\*/#', '', $script );

return str_replace(
array_keys( $this->replacements ),
array_values( $this->replacements ),
$script
);
}
}
Loading