Skip to content

Add support for AMP dev mode #505

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

Merged
merged 13 commits into from
Nov 20, 2019
Merged

Add support for AMP dev mode #505

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bf18d79
Add initial support for AMP dev mode
westonruter Sep 8, 2019
b50a200
Fix typo in phpdoc tag
westonruter Sep 21, 2019
368dd24
Merge branch 'develop' of github.com:google/site-kit-wp into add/amp-…
westonruter Oct 27, 2019
b822bd4
Short-circuit AMP dev mode integration if not in dev mode
westonruter Oct 27, 2019
62e5bba
Merge branch 'add/amp-dev-mode-support' of github.com:google/site-kit…
felixarntz Nov 20, 2019
7561d0d
Render full Site Kit admin bar menu markup if AMP dev mode is enabled.
felixarntz Nov 20, 2019
69831d4
Bundle wp-escape-html and wp-is-shallow-equal since they are dependen…
felixarntz Nov 20, 2019
545db10
Update external WP asset versions to those actually bundled.
felixarntz Nov 20, 2019
f7f96a0
Merge branch 'develop' into add/amp-dev-mode-support
felixarntz Nov 20, 2019
3e4948d
Merge branch 'develop' into add/amp-dev-mode-support
felixarntz Nov 20, 2019
ce61c12
Merge branch 'develop' into add/amp-dev-mode-support
westonruter Nov 20, 2019
79d344c
Use traditional array syntax for consistency.
felixarntz Nov 20, 2019
1db67b9
Merge branch 'develop' into add/amp-dev-mode-support
felixarntz Nov 20, 2019
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
20 changes: 19 additions & 1 deletion includes/Core/Admin_Bar/Admin_Bar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ function( $wp_admin_bar ) {
$this->assets->enqueue_asset( 'googlesitekit_adminbar_css' );

if ( $this->context->is_amp() ) {
return;
if ( ! function_exists( 'amp_is_dev_mode' ) || ! amp_is_dev_mode() ) {
// AMP Dev Mode support was added in v1.4, and if it is not enabled then short-circuit since scripts will be invalid.
return;
}
add_filter( 'amp_dev_mode_element_xpaths', [ $this, 'add_amp_dev_mode' ] );
}

// Enqueue scripts.
Expand All @@ -92,6 +96,20 @@ function( $wp_admin_bar ) {
add_action( 'wp_enqueue_scripts', $admin_bar_callback, 40 );
}

/**
* Add data-ampdevmode attributes to the elements that need it.
*
* @see \Google\Site_Kit\Core\Assets\Assets::get_assets() The 'googlesitekit' string is added to all inline scripts.
* @see \Google\Site_Kit\Core\Assets\Assets::add_amp_dev_mode_attributes() The data-ampdevmode attribute is added to registered scripts/styles here.
*
* @param string[] $xpath_queries XPath queries for elements that should get the data-ampdevmode attribute.
* @return string[] XPath queries.
*/
public function add_amp_dev_mode( $xpath_queries ) {
$xpath_queries[] = '//script[ contains( text(), "googlesitekit" ) ]';
Copy link
Member

@felixarntz felixarntz Oct 27, 2019

Choose a reason for hiding this comment

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

Just to clarify, this is only for inline scripts then right? The non-inline scripts receive the attribute in their own *_loader_tag hooks I see.

Adding a random googlesitekit comment to all these scripts is not so great. Is there not a way to add the data-ampdevmode attribute to those inline scripts with a filter too?

Copy link
Contributor Author

@westonruter westonruter Oct 27, 2019

Choose a reason for hiding this comment

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

Just to clarify, this is only for inline scripts then right? The non-inline scripts receive the attribute in their own *_loader_tag hooks I see.

Yes, that is correct.

Adding a random googlesitekit comment to all these scripts is not so great. Is there not a way to add the data-ampdevmode attribute to those inline scripts with a filter too?

It's not awesome, but it is effective and somewhat nice for debugging to locate and identify scripts coming from Site Kit.

I did try to find a way to filter the inline scripts to inject this data-amp-dev-mode attribute, but I was unable to. The extra scripts get printed in \WP_Scripts::print_extra_script() which unfortunately has no filter like script_loader_tag.

return $xpath_queries;
}

/**
* Render the Adminbar button.
*
Expand Down
47 changes: 41 additions & 6 deletions includes/Core/Assets/Assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,41 @@ private function register_assets() {
foreach ( $assets as $asset ) {
$asset->register();
}

$this->add_amp_dev_mode_attributes( $assets );
}

/**
* Add data-ampdevmode attributes to assets.
*
* @todo What about dependencies?
*
* @param Asset[] $assets Assets.
*/
private function add_amp_dev_mode_attributes( $assets ) {
add_filter(
'script_loader_tag',
function ( $tag, $handle ) use ( $assets ) {
if ( $this->context->is_amp() && isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Script ) {
Copy link
Contributor Author

@westonruter westonruter Oct 27, 2019

Choose a reason for hiding this comment

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

This apparently needs to look to also look to see if the $handle is a dependency of any $assets as well.

Copy link
Member

@felixarntz felixarntz Oct 28, 2019

Choose a reason for hiding this comment

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

We can probably add a get_dependency_handles() method to Google\Site_Kit\Core\Assets\Asset, which we can then leverage here to check that.

Copy link
Collaborator

@aaemnnosttv aaemnnosttv Nov 20, 2019

Choose a reason for hiding this comment

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

Out of curiosity, why would the amp plugin not just add the attribute to all style and script tags? Why only whitelist Site Kit assets and dependencies? If needed, why wouldn't the amp plugin add the attributes and let other plugins hook in to whitelist its assets rather than each plugin reimplementing this itself?

Copy link
Member

@felixarntz felixarntz Nov 20, 2019

Choose a reason for hiding this comment

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

The attribute should only be added to JS that is related to functionality not relevant to the frontend (and which thus would not actually break AMP compatibility of the frontend, as long as you're logged-out). That's why it needs to be conservative, any plugin that has JS-powered admin bar functionality needs to explicitly declare that.

Regarding addition of the attributes, that's a good point. @westonruter Maybe the AMP plugin could include a central solution for adding the attribute that scripts/stylesheets could leverage via e.g. a wp_*_add_data( ... )?

Copy link
Contributor Author

@westonruter westonruter Nov 20, 2019

Choose a reason for hiding this comment

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

Note that any script or stylesheet that has admin-bar as a dependency will automatically get the dev mode attribute. The issue is with scripts/styles that do not have this explicit admin bar linkage, such as dependencies on general libraries.

Maybe the AMP plugin could include a central solution for adding the attribute that scripts/stylesheets could leverage via e.g. a wp_*_add_data( ... )?

I'm not sure I understand. Are you saying you could do something like:

wp_script_add_data( 'lodash', 'ampdevmode', true );

And that this could be used as a dev mode signal equivalent to dependence on the admin-bar script? Then the AMP plugin could include a script_loader_tag filter which adds the data-ampdevmode attribute automatically to any script that has the ampdevmode data or which depends on a script that has this attribute?

This could also be used to automatically add the comment for inline scripts as well. Whereas right now we are manually adding /* googlesitekit */ to the inline scripts, there could be a wp_print_scripts action which loops over all scripts prior to printing and any script which contains the ampdevmode data could also then do:

wp_add_inline_script( $handle, '/*ampdevmode*/' );

And this pattern could be automatically added to the list of amp_dev_mode_element_xpaths.

Does this sound right?

Copy link
Member

@felixarntz felixarntz Dec 6, 2019

Choose a reason for hiding this comment

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

@westonruter Yes, that would be great - really like including inline script support as well.

Copy link
Contributor Author

@westonruter westonruter Dec 20, 2019

Choose a reason for hiding this comment

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

Work in progress: ampproject/amp-wp#4001

$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' data-ampdevmode', $tag );
}
return $tag;
},
10,
2
);

add_filter(
'style_loader_tag',
function ( $tag, $handle ) use ( $assets ) {
if ( $this->context->is_amp() && isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Stylesheet ) {
$tag = preg_replace( '/(?<=<link)(?=\s|>)/i', ' data-ampdevmode', $tag );
}
return $tag;
},
10,
2
);
}

/**
Expand All @@ -248,7 +283,7 @@ private function enqueue_minimal_admin_script() {
*
* @since 1.0.0
*
* @return array Associative array of asset $handle => $instance pairs.
* @return Asset[] Associative array of asset $handle => $instance pairs.
*/
private function get_assets() {
if ( $this->assets ) {
Expand Down Expand Up @@ -280,7 +315,7 @@ private function get_assets() {
'dependencies' => array( 'sitekit-vendor' ),
'post_register' => function( $handle ) use ( $base_url ) {
$url_polyfill = (
'( typeof URL === \'function\') || ' .
'/*googlesitekit*/ ( typeof URL === \'function\') || ' .
'document.write( \'<script src="' . // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
$base_url . 'js/externals/wp-polyfill-url.js' .
'"></scr\' + \'ipt>\' );'
Expand Down Expand Up @@ -639,7 +674,7 @@ private function get_external_assets() {
'version' => '4.17.11',
'fallback' => true,
'post_register' => function( $handle ) {
wp_add_inline_script( $handle, 'window.lodash = window.lodash || _.noConflict(); window.lodash_load = true;' );
wp_add_inline_script( $handle, '/*googlesitekit*/ window.lodash = window.lodash || _.noConflict(); window.lodash_load = true;' );
},
)
),
Expand Down Expand Up @@ -681,7 +716,7 @@ private function get_external_assets() {
'window.FormData && window.FormData.prototype.keys' => $base_url . 'js/externals/wp-polyfill-formdata.js', // phpcs:ignore WordPress.Arrays.MultipleStatementAlignment
'Element.prototype.matches && Element.prototype.closest' => $base_url . 'js/externals/wp-polyfill-element-closest.js', // phpcs:ignore WordPress.Arrays.MultipleStatementAlignment
);
$polyfill_scripts = '';
$polyfill_scripts = '/*googlesitekit*/';
foreach ( $inline_polyfill_tests as $test => $script ) { // phpcs:ignore Generic.WhiteSpace.ScopeIndent.IncorrectExact
$polyfill_scripts .= (
'( ' . $test . ' ) || ' .
Expand Down Expand Up @@ -752,15 +787,15 @@ private function get_external_assets() {
wp_add_inline_script(
$handle,
sprintf(
'wp.apiFetch.use( wp.apiFetch.createNonceMiddleware( "%s" ) );',
'/*googlesitekit*/ wp.apiFetch.use( wp.apiFetch.createNonceMiddleware( "%s" ) );',
( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' )
),
'after'
);
wp_add_inline_script(
$handle,
sprintf(
'wp.apiFetch.use( wp.apiFetch.createRootURLMiddleware( "%s" ) );',
'/*googlesitekit*/ wp.apiFetch.use( wp.apiFetch.createRootURLMiddleware( "%s" ) );',
esc_url_raw( get_rest_url() )
),
'after'
Expand Down