-
Notifications
You must be signed in to change notification settings - Fork 286
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
Add support for AMP dev mode #505
Changes from all commits
bf18d79
b50a200
368dd24
b822bd4
62e5bba
7561d0d
69831d4
545db10
f7f96a0
3e4948d
ce61c12
79d344c
1db67b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This apparently needs to look to also look to see if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably add a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
); | ||
} | ||
|
||
/** | ||
|
@@ -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 ) { | ||
|
@@ -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>\' );' | ||
|
@@ -630,10 +665,10 @@ private function get_external_assets() { | |
'lodash', | ||
array( | ||
'src' => $base_url . 'vendor/lodash' . $suffix . '.js', | ||
'version' => '4.17.11', | ||
'version' => '4.17.15', | ||
'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;' ); | ||
}, | ||
) | ||
), | ||
|
@@ -649,15 +684,15 @@ private function get_external_assets() { | |
'react', | ||
array( | ||
'src' => $base_url . 'vendor/react' . $react_suffix . '.js', | ||
'version' => '16.8.5', | ||
'version' => '16.11.0', | ||
'fallback' => true, | ||
) | ||
), | ||
new Script( | ||
'react-dom', | ||
array( | ||
'src' => $base_url . 'vendor/react-dom' . $react_suffix . '.js', | ||
'version' => '16.8.5', | ||
'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 = ''; | ||
$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.2.0', | ||
'version' => '2.6.0', | ||
'fallback' => true, | ||
) | ||
), | ||
new Script( | ||
'wp-element', | ||
array( | ||
'src' => $base_url . 'js/externals/element.js', | ||
'version' => '2.3.0', | ||
'version' => '2.8.2', | ||
'fallback' => true, | ||
) | ||
), | ||
new Script( | ||
'wp-dom-ready', | ||
array( | ||
'src' => $base_url . 'js/externals/domReady.js', | ||
'version' => '2.2.0', | ||
'version' => '2.5.1', | ||
'fallback' => true, | ||
) | ||
), | ||
new Script( | ||
'wp-i18n', | ||
array( | ||
'src' => $base_url . 'js/externals/i18n.js', | ||
'version' => '3.3.0', | ||
'version' => '3.6.1', | ||
'fallback' => true, | ||
) | ||
), | ||
new Script( | ||
'wp-url', | ||
array( | ||
'src' => $base_url . 'js/externals/url.js', | ||
'version' => '2.3.3', | ||
'version' => '2.8.2', | ||
'fallback' => true, | ||
) | ||
), | ||
new Script( | ||
'wp-api-fetch', | ||
array( | ||
'src' => $base_url . 'js/externals/apiFetch.js', | ||
'version' => '2.2.8', | ||
'version' => '3.6.4', | ||
'fallback' => true, | ||
'post_register' => function( $handle ) { | ||
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' | ||
|
@@ -758,7 +809,7 @@ private function get_external_assets() { | |
'wp-compose', | ||
array( | ||
'src' => $base_url . 'js/externals/compose.js', | ||
'version' => '3.2.0', | ||
'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_tag
hooks I see.Adding a random
googlesitekit
comment to all these scripts is not so great. Is there not a way to add thedata-ampdevmode
attribute 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-mode
attribute, but I was unable to. The extra scripts get printed in\WP_Scripts::print_extra_script()
which unfortunately has no filter likescript_loader_tag
.