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

Remove scripts for components that were not detected in output buffer #3705

Merged
merged 11 commits into from
Nov 15, 2019
Merged

Remove scripts for components that were not detected in output buffer #3705

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cee8d58
Remove scripts for components that were not detected in output buffer
schlessera Nov 10, 2019
7d1bce5
Merge branch 'develop' of github.com:ampproject/amp-wp into 3489-remo…
westonruter Nov 13, 2019
8744c89
Eliminate extracting distict also_requires_tag_warning from spec
westonruter Nov 13, 2019
dba4ead
Normalize required_usage in extension_spec
westonruter Nov 13, 2019
2f9f8b9
Add required_usage info as script data and use to determine which scr…
westonruter Nov 13, 2019
d9469bf
Merge branch 'develop' of github.com:ampproject/amp-wp into 3489-remo…
westonruter Nov 15, 2019
879cb17
Improve handling of versions in extension_spec
westonruter Nov 15, 2019
17d03ad
Remove unused deprecated_allow_duplicates from extension_spec
westonruter Nov 15, 2019
9956c90
Implement method to get all extension_specs
westonruter Nov 15, 2019
16f35b4
Cache the collection of the extension_specs
westonruter Nov 15, 2019
617d17d
Remove todo
schlessera Nov 15, 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
37 changes: 27 additions & 10 deletions bin/amphtml-update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,11 +463,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 +488,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 +513,28 @@ 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' )

tag_rules['extension_spec'] = extension_spec

if tag_spec.HasField('mandatory'):
Expand Down
45 changes: 25 additions & 20 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,38 +464,43 @@ function amp_register_default_scripts( $wp_scripts ) {
]
);

// Get all AMP components as defined in the spec.
$extensions = [];
// Register all AMP components as defined in the spec.
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( $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

The Python parser should probably be smarter about extracting all of the extension specs up front to then provide them in a single call, like AMP_Allowed_Tags_Generated::get_extension_specs().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should at one point rethink the way the spec is parsed. I've created an issue for that: #3730

Copy link
Member

Choose a reason for hiding this comment

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

I love that.

Copy link
Member

Choose a reason for hiding this comment

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

While we plan out improving how the spec is parsed, I've implemented a AMP_Allowed_Tags_Generated::get_extension_specs() helper to improve things in the meantime. See 9956c90.

continue;
}
}

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';
}
$extension_spec = $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec'];
$versions = array_filter(
$extension_spec['version'],
function ( $version ) {
// @todo Why are we including the latest version in the generated spec in the first place? We can just include the highest number version.
return 'latest' !== $version;
westonruter marked this conversation as resolved.
Show resolved Hide resolved
}
);

foreach ( $extensions as $extension => $version ) {
$src = sprintf(
'https://cdn.ampproject.org/v0/%s-%s.js',
$extension,
$version
$extension_spec['name'],
array_pop( $versions )
);

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

$wp_scripts->add_data( $extension_spec['name'], 'amp_requires_usage', ! empty( $extension_spec['requires_usage'] ) );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super happy with what I did here. The post-processor shouldn't be looking at the wp_scripts() object to determine later if this requires usage. It would be better if the extension_specs were obtained again during post-processing and the required_usage was obtained at that point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about even moving the entire registration to a point where we know what is needed and what is not? That would avoid registering scripts we don't need, and you could have all checks against the spec in one place...

Copy link
Member

Choose a reason for hiding this comment

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

These scripts are needing to be registered in WP_Scripts to ensure that they will be available to enqueue for AMP components used outside of AMP pages. For example, Newspack is using amp-carousel in its Carousel block and the block will wp_enqueue_script( 'amp-carousel' ) which will ensure it works in AMP pages and non-AMP pages alike. (Granted, this is not guaranteed to work prior to Bento AMP, but still, it works most of the time and is better than a blank element.)

}

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
13 changes: 12 additions & 1 deletion includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1727,7 +1727,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 +1745,17 @@ 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.
$superfluous_script_handles = array_diff(
array_keys( $amp_scripts ),
array_merge( $script_handles, [ 'amp-runtime' ] )
);
foreach ( $superfluous_script_handles as $superfluous_script_handle ) {
if ( true === wp_scripts()->get_data( $superfluous_script_handle, 'amp_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