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.

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

Consume bundles.config.extensions.json to amend TagWithExtensionSpec classes with extension version meta #297

Merged
merged 11 commits into from
Aug 19, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 30, 2021

The AMP project contains extension version information in a bundles config file: https://github.com/ampproject/amphtml/blob/main/build-system/compile/bundles.config.extensions.json

This information is complementary to what is located in the validator spec. It has key information including what specific version the the latest version corresponds and what versions are available as Bento components.

This PR augments TagWithExtensionSpec to include this additional extension version information.

It still feels somewhat not-ideal that the the extension version information gets split apart into multiple TagWithExtensionSpec classes. For example, amp-twitter scripts have a ScriptAmpTwitter and a ScriptAmpTwitter2 class. The former is the classic AMP component version whereas the latter is the Bento version. Both of these classes have a LATEST_VERSION which points to 0.1, and the versions of the former include 0.1 but the later includes just 1.0. So this may not be the most ergonomic way to store/access this information.

Feel free to take over this PR and make changes that seem best to you. Maybe we shouldn't be extending TagWithExtensionSpec classes in the first place and it would be better to represent this information in a separate set of classes.

@westonruter westonruter changed the title Add Bento awareness Consume bundles.config.extensions.json to amend TagWithExtensionSpec classes with extension version meta Jul 30, 2021
@westonruter westonruter marked this pull request as ready for review July 30, 2021 05:44
@westonruter westonruter requested a review from schlessera July 30, 2021 05:44
@schlessera schlessera added this to the 0.7.0 milestone Aug 19, 2021
@schlessera schlessera merged commit 71432b9 into main Aug 19, 2021
@schlessera schlessera deleted the add/bento-meta branch August 19, 2021 15:51
@schlessera
Copy link
Collaborator

@westonruter:

It still feels somewhat not-ideal that the the extension version information gets split apart into multiple TagWithExtensionSpec classes. For example, amp-twitter scripts have a ScriptAmpTwitter and a ScriptAmpTwitter2 class. The former is the classic AMP component version whereas the latter is the Bento version. Both of these classes have a LATEST_VERSION which points to 0.1, and the versions of the former include 0.1 but the later includes just 1.0. So this may not be the most ergonomic way to store/access this information.

I added the byAggregateExtensionSpec() retrieval now which returns an aggregate tag when multiple tags match. The information you can get via the aggregate is of course limited, but it can for example return the combined version meta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants