Skip to content

Commit

Permalink
Merge pull request #3705 from ampproject/3489-remove-duplicate-amp-sc…
Browse files Browse the repository at this point in the history 
…ripts

Remove scripts for components that were not detected in output buffer
  • Loading branch information
@westonruter
westonruter authored Nov 15, 2019
2 parents 3a0c068 + 617d17d commit 451dd22
Show file tree
Hide file tree
Showing 6 changed files with 319 additions and 293 deletions.
75 changes: 65 additions & 10 deletions bin/amphtml-update.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,29 @@ def GenerateFooterPHP(out):
return self::$allowed_tags;
}
/**
* Get extension specs.
*
* @since 1.5
* @internal
* @return array Extension specs, keyed by extension name.
*/
public static function get_extension_specs() {
static $extension_specs = [];
if ( ! empty( $extension_specs ) ) {
return $extension_specs;
}
foreach ( self::get_allowed_tag( 'script' ) as $script_spec ) {
if ( isset( $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec'] ) ) {
$extension_specs[ $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'] ] = $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec'];
}
}
return $extension_specs;
}
/**
* Get allowed tag.
*
Expand Down Expand Up @@ -463,11 +486,20 @@ def GetTagRules(tag_spec):
also_requires_tag_list.append(UnicodeEscape(also_requires_tag))
tag_rules['also_requires_tag'] = also_requires_tag_list

requires_extension_list = set()
if hasattr(tag_spec, 'requires_extension') and len( tag_spec.requires_extension ) != 0:
requires_extension_list = []
for requires_extension in tag_spec.requires_extension:
requires_extension_list.append(requires_extension)
tag_rules['requires_extension'] = requires_extension_list
requires_extension_list.add(requires_extension)

if hasattr(tag_spec, 'also_requires_tag_warning') and len( tag_spec.also_requires_tag_warning ) != 0:
for also_requires_tag_warning in tag_spec.also_requires_tag_warning:
matches = re.search( r'(amp-\S+) extension .js script', also_requires_tag_warning )
if not matches:
raise Exception( 'Unexpected also_requires_tag_warning format: ' + also_requires_tag_warning )
requires_extension_list.add(matches.group(1))

if len( requires_extension_list ) > 0:
tag_rules['requires_extension'] = list( requires_extension_list )

if hasattr(tag_spec, 'reference_points') and len( tag_spec.reference_points ) != 0:
tag_reference_points = {}
Expand All @@ -479,12 +511,6 @@ def GetTagRules(tag_spec):
if len( tag_reference_points ) > 0:
tag_rules['reference_points'] = tag_reference_points

if hasattr(tag_spec, 'also_requires_tag_warning') and len( tag_spec.also_requires_tag_warning ) != 0:
also_requires_tag_warning_list = []
for also_requires_tag_warning in tag_spec.also_requires_tag_warning:
also_requires_tag_warning_list.append(also_requires_tag_warning)
tag_rules['also_requires_tag_warning'] = also_requires_tag_warning_list

if tag_spec.disallowed_ancestor:
disallowed_ancestor_list = []
for disallowed_ancestor in tag_spec.disallowed_ancestor:
Expand All @@ -510,14 +536,43 @@ def GetTagRules(tag_spec):
return None

if tag_spec.HasField('extension_spec'):
extension_spec = {}
# See https://github.com/ampproject/amphtml/blob/e37f50d/validator/validator.proto#L430-L454
ERROR = 1
NONE = 3
extension_spec = {
'requires_usage': 1 # (ERROR=1)
}
for field in tag_spec.extension_spec.ListFields():
if isinstance(field[1], (list, google.protobuf.internal.containers.RepeatedScalarFieldContainer, google.protobuf.pyext._message.RepeatedScalarContainer)):
extension_spec[ field[0].name ] = []
for val in field[1]:
extension_spec[ field[0].name ].append( val )
else:
extension_spec[ field[0].name ] = field[1]

# Normalize ERROR and GRANDFATHERED as true, since we control which scripts are added (or removed) from the output.
extension_spec['requires_usage'] = ( extension_spec['requires_usage'] != 3 ) # NONE=3

if 'version' not in extension_spec:
raise Exception( 'Missing required version field' )
if 'name' not in extension_spec:
raise Exception( 'Missing required name field' )

# Get the versions and sort.
versions = set( extension_spec['version'] )
versions.remove( 'latest' )
extension_spec['version'] = sorted( versions, key=lambda version: map(int, version.split('.') ) )

# Unused since amp_filter_script_loader_tag() and \AMP_Tag_And_Attribute_Sanitizer::get_rule_spec_list_to_validate() just hard-codes the check for amp-mustache.
if 'extension_type' in extension_spec:
del extension_spec['extension_type']

if 'deprecated_version' in extension_spec:
del extension_spec['deprecated_version']

if 'deprecated_allow_duplicates' in extension_spec:
del extension_spec['deprecated_allow_duplicates']

tag_rules['extension_spec'] = extension_spec

if tag_spec.HasField('mandatory'):
Expand Down
39 changes: 16 additions & 23 deletions includes/amp-helper-functions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,39 +464,30 @@ function amp_register_default_scripts( $wp_scripts ) {
]
);

// Get all AMP components as defined in the spec.
$extensions = [];
foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'script' ) as $script_spec ) {
if ( isset( $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'], $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['version'] ) ) {
$versions = $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['version'];
array_pop( $versions );
$extensions[ $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'] ] = array_pop( $versions );
}
}

if ( isset( $extensions['amp-carousel'] ) ) {
/*
* The 0.2 version of amp-carousel depends on the amp-base-carousel component, but this is still experimental.
* Also, the validator spec does not currently specify what base dependencies a given component has.
* @todo Revisit once amp-base-carousel is no longer experimental. Add support for obtaining a list of extensions that depend on other extensions to include in the script dependencies when registering below.
*/
$extensions['amp-carousel'] = '0.1';
}

foreach ( $extensions as $extension => $version ) {
// Register all AMP components as defined in the spec.
foreach ( AMP_Allowed_Tags_Generated::get_extension_specs() as $extension_name => $extension_spec ) {
$src = sprintf(
'https://cdn.ampproject.org/v0/%s-%s.js',
$extension,
$version
$extension_name,
end( $extension_spec['version'] )
);

$wp_scripts->add(
$extension,
$extension_name,
$src,
[ 'amp-runtime' ],
null
);
}

if ( $wp_scripts->query( 'amp-carousel', 'registered' ) ) {
/*
* The 0.2 version of amp-carousel depends on the amp-base-carousel component, but this is still experimental.
* Also, the validator spec does not currently specify what base dependencies a given component has.
* @todo Revisit once amp-base-carousel is no longer experimental. Add support for obtaining a list of extensions that depend on other extensions to include in the script dependencies when registering below.
*/
$wp_scripts->registered['amp-carousel']->src = 'https://cdn.ampproject.org/v0/amp-carousel-0.1.js';
}
}

/**
Expand Down Expand Up @@ -576,6 +567,8 @@ function amp_filter_script_loader_tag( $tag, $handle ) {
/*
* Per the spec, "Most extensions are custom-elements." In fact, there is only one custom template. So we hard-code it here.
*
* This could also be derived by looking at the extension_type in the extension_spec.
*
* @link https://github.com/ampproject/amphtml/blob/cd685d4e62153557519553ffa2183aedf8c93d62/validator/validator.proto#L326-L328
* @link https://github.com/ampproject/amphtml/blob/cd685d4e62153557519553ffa2183aedf8c93d62/extensions/amp-mustache/validator-amp-mustache.protoascii#L27
*/
Expand Down
15 changes: 13 additions & 2 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1534,7 +1534,6 @@ public static function filter_admin_bar_script_loader_tag( $tag, $handle ) {
* @link https://www.ampproject.org/docs/reference/spec#required-markup
* @link https://amp.dev/documentation/guides-and-tutorials/optimize-and-measure/optimize_amp/
* @todo All of this might be better placed inside of a sanitizer.
* @todo Consider removing any scripts that are not among the $script_handles.
*
* @param DOMDocument $dom Document.
* @param string[] $script_handles AMP script handles for components identified during output buffering.
Expand Down Expand Up @@ -1727,7 +1726,7 @@ public static function ensure_required_markup( DOMDocument $dom, $script_handles
$script->parentNode->removeChild( $script ); // So we can move it.
}

// Create scripts for any components discovered from output buffering.
// Create scripts for any components discovered from output buffering that are missing.
foreach ( array_diff( $script_handles, array_keys( $amp_scripts ) ) as $missing_script_handle ) {
if ( ! wp_script_is( $missing_script_handle, 'registered' ) ) {
continue;
Expand All @@ -1745,6 +1744,18 @@ public static function ensure_required_markup( DOMDocument $dom, $script_handles
$amp_scripts[ $missing_script_handle ] = AMP_DOM_Utils::create_node( $dom, 'script', $attrs );
}

// Remove scripts that had already been added but couldn't be detected from output buffering.
$extension_specs = AMP_Allowed_Tags_Generated::get_extension_specs();
$superfluous_script_handles = array_diff(
array_keys( $amp_scripts ),
array_merge( $script_handles, [ 'amp-runtime' ] )
);
foreach ( $superfluous_script_handles as $superfluous_script_handle ) {
if ( ! empty( $extension_specs[ $superfluous_script_handle ]['requires_usage'] ) ) {
unset( $amp_scripts[ $superfluous_script_handle ] );
}
}

/* phpcs:ignore Squiz.PHP.CommentedOutCode.Found
*
* "2. Next, preload the AMP runtime v0.js <script> tag with <link as=script href=https://cdn.ampproject.org/v0.js rel=preload>.
Expand Down
Loading

0 comments on commit 451dd22

Please sign in to comment.