Add support for AMP dev mode #505
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 ) { | ||
|
There was a problem hiding this comment. This apparently needs to look to also look to see if the There was a problem hiding this comment. We can probably add a There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 There was a problem hiding this comment. Note that any script or stylesheet that has
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 This could also be used to automatically add the comment for inline scripts as well. Whereas right now we are manually adding wp_add_inline_script( $handle, '/*ampdevmode*/' );And this pattern could be automatically added to the list of Does this sound right? There was a problem hiding this comment. @westonruter Yes, that would be great - really like including inline script support as well. There was a problem hiding this comment. 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 | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -248,7 +283,7 @@ private function enqueue_minimal_admin_script() { | |
| * | ||
| * @since 1.0.0 | ||
| * | ||
| * @return Asset[] Associative array of asset $handle => $instance pairs. | ||
| */ | ||
| private function get_assets() { | ||
| if ( $this->assets ) { | ||
|
|
@@ -280,7 +315,7 @@ private function get_assets() { | |
| 'dependencies' => array( 'sitekit-vendor' ), | ||
| 'post_register' => function( $handle ) use ( $base_url ) { | ||
| $url_polyfill = ( | ||
| '/*googlesitekit*/ ( typeof URL === \'function\') || ' . | ||
| 'document.write( \'<script src="' . // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript | ||
| $base_url . 'js/externals/wp-polyfill-url.js' . | ||
| '"></scr\' + \'ipt>\' );' | ||
|
|
@@ -630,10 +665,10 @@ private function get_external_assets() { | |
| 'lodash', | ||
| array( | ||
| 'src' => $base_url . 'vendor/lodash' . $suffix . '.js', | ||
| 'version' => '4.17.15', | ||
| 'fallback' => true, | ||
| 'post_register' => function( $handle ) { | ||
| wp_add_inline_script( $handle, '/*googlesitekit*/ window.lodash = window.lodash || _.noConflict(); window.lodash_load = true;' ); | ||
| }, | ||
| ) | ||
| ), | ||
|
|
@@ -649,15 +684,15 @@ private function get_external_assets() { | |
| 'react', | ||
| array( | ||
| 'src' => $base_url . 'vendor/react' . $react_suffix . '.js', | ||
| 'version' => '16.11.0', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'react-dom', | ||
| array( | ||
| 'src' => $base_url . 'vendor/react-dom' . $react_suffix . '.js', | ||
| 'version' => '16.11.0', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
|
|
@@ -675,7 +710,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 = '/*googlesitekit*/'; | ||
| foreach ( $inline_polyfill_tests as $test => $script ) { // phpcs:ignore Generic.WhiteSpace.ScopeIndent.IncorrectExact | ||
| $polyfill_scripts .= ( | ||
| '( ' . $test . ' ) || ' . | ||
|
|
@@ -688,65 +723,81 @@ private function get_external_assets() { | |
| }, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-escape-html', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/escapeHtml.js', | ||
| 'version' => '1.5.1', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-is-shallow-equal', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/isShallowEqual.js', | ||
| 'version' => '1.6.1', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-hooks', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/hooks.js', | ||
| 'version' => '2.6.0', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-element', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/element.js', | ||
| 'version' => '2.8.2', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-dom-ready', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/domReady.js', | ||
| 'version' => '2.5.1', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-i18n', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/i18n.js', | ||
| 'version' => '3.6.1', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-url', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/url.js', | ||
| 'version' => '2.8.2', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
| new Script( | ||
| 'wp-api-fetch', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/apiFetch.js', | ||
| 'version' => '3.6.4', | ||
| 'fallback' => true, | ||
| 'post_register' => function( $handle ) { | ||
| wp_add_inline_script( | ||
| $handle, | ||
| sprintf( | ||
| '/*googlesitekit*/ wp.apiFetch.use( wp.apiFetch.createNonceMiddleware( "%s" ) );', | ||
| ( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' ) | ||
| ), | ||
| 'after' | ||
| ); | ||
| wp_add_inline_script( | ||
| $handle, | ||
| sprintf( | ||
| '/*googlesitekit*/ wp.apiFetch.use( wp.apiFetch.createRootURLMiddleware( "%s" ) );', | ||
| esc_url_raw( get_rest_url() ) | ||
| ), | ||
| 'after' | ||
|
|
@@ -758,7 +809,7 @@ private function get_external_assets() { | |
| 'wp-compose', | ||
| array( | ||
| 'src' => $base_url . 'js/externals/compose.js', | ||
| 'version' => '3.7.2', | ||
| 'fallback' => true, | ||
| ) | ||
| ), | ||
|
|
||
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.
Just to clarify, this is only for inline scripts then right? The non-inline scripts receive the attribute in their own
*_loader_taghooks I see.Adding a random
googlesitekitcomment to all these scripts is not so great. Is there not a way to add thedata-ampdevmodeattribute to those inline scripts with a filter too?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, that is correct.
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-modeattribute, but I was unable to. The extra scripts get printed in\WP_Scripts::print_extra_script()which unfortunately has no filter likescript_loader_tag.