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.

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

Jump to bottom

Facilitate marking script/style dependencies as being covered by AMP dev mode #4598

Closed
westonruter opened this issue Apr 16, 2020 · 1 comment · May be fixed by #4001
Closed

Facilitate marking script/style dependencies as being covered by AMP dev mode #4598

westonruter opened this issue Apr 16, 2020 · 1 comment · May be fixed by #4001
Labels
Developer Tools Enhancement New feature or improvement of an existing one Groomed P1 Medium priority Punted Validation WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

Feature description

Originally discussed in google/site-kit-wp#505 (comment).

The Site Kit plugin currently marks scripts for being as part of dev mode by:

  1. Adding /*googlesitekit*/ to the inline script content.
  2. Adding a //script[ contains( text(), "googlesitekit" ) ] to the amp_dev_mode_element_xpaths.

This works but it is not the best developer experience, nor is it the most robust. It depends on injecting a unique string into the scripts which serves no purpose other than for marking content for dev mode. It also slows down the post-processor a bit by requiring XPath to query for the script elements to add the data-ampdevmode attribute to.

For non-inline scripts, marking scripts for dev mode requires either making it a dependency of admin-bar, or to add script_loader_tag/style_loader_tag filters to inject the data-ampdevmode attribute. This is also not the best developer experience.

A much better experience for flagging scripts for dev mode would by to attach data to the script (or style). For example:

wp_script_add_data( 'escript', 'ampdevmode', true );
wp_style_add_data( 'estyle', 'ampdevmode', true );

When this ampdevmode data is attached to a dependency, this can be looked for in the script_loader_tag/style_loader_tag as a signal to add the data-ampdevmode attribute, instead of just relying on the handle being a dependency of admin-bar.

Not all script and style tags are filterable, however. Namely, styles added via wp_add_inline_style() and scripts added via wp_localize_script() get printed directly and are not filterable. (Scripts added via wp_add_inline_script() do get passed through the script_loader_tag filter). For these, we'll need rely on the post-processor to inject the data-ampdevmode attributes. This entails adding new XPath queries via the amp_dev_mode_element_xpaths filter for AMP_Dev_Mode_Sanitizer.

In the end, all scripts and styles—whether external or inline—should automatically have the data-ampdevmode attribute added to them when the underlying registered script/style dependency has the ampdevmode data added to them.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry
@westonruter westonruter mentioned this issue Apr 16, 2020
Draft
4 tasks
@amedina amedina changed the title Facilitate marking script/style dependencies as being under AMP dev mode Facilitate marking script/style dependencies as being covered AMP dev mode May 14, 2020
@amedina amedina changed the title Facilitate marking script/style dependencies as being covered AMP dev mode Facilitate marking script/style dependencies as being covered by AMP dev mode May 14, 2020
@amedina amedina added the Enhancement New feature or improvement of an existing one label May 14, 2020
@kmyram kmyram added this to the v1.6 milestone May 27, 2020
@amedina amedina added the P1 Medium priority label Jun 10, 2020
@westonruter westonruter modified the milestones: v1.6, v1.7 Jun 28, 2020
@kmyram kmyram added the Groomed label Jun 30, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
This was referenced Sep 1, 2020
Merged
Closed
@westonruter
Copy link
Member Author

In johnbillion/query-monitor#535 (comment), @johnbillion also suggested using a filter for mark scripts/styles for AMP Dev Mode rather than adding data directly to the assets:

add_filter( 'amp_dev_mode_scripts', function( $scripts ) {
	$scripts[] = 'query-monitor';
	return $scripts;
} );

add_filter( 'amp_dev_mode_styles', function( $styles ) {
	$styles[] = 'query-monitor';
	return $styles;
} );

A benefit here is we don't have to worry about the script/style being registered first, as otherwise wp_script_add_data() and wp_style_add_data() will short-circuit.

@westonruter westonruter self-assigned this Jan 15, 2021
@westonruter westonruter modified the milestones: v2.1, v2.2 Feb 11, 2021
@westonruter westonruter removed their assignment Jun 18, 2021
@westonruter westonruter modified the milestones: v2.2, v2.3 Nov 22, 2021
@westonruter westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021
@westonruter westonruter removed this from the v2.4 milestone Apr 14, 2022
@westonruter westonruter closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2023
@github-project-automation github-project-automation bot moved this to To Do in Ongoing Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Developer Tools Enhancement New feature or improvement of an existing one Groomed P1 Medium priority Punted Validation WS:Core Work stream for Plugin core
Projects
Ongoing
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Introduce ampdevmode data flag on script/style dependencies ampproject/amp-wp
4 participants
@westonruter @amedina @kmyram @dhaval-parekh