Bento: Validate Facebook components #35395

caroqliu · 2021-07-26T15:24:47Z

This PR introduces validations for the 1.0 versions of amp-facebook, amp-facebook-comments, amp-facebook-like, and amp-facebook-page, all of which are provided by the amp-facebook-1.0.js extension script.

In particular we want to guard for the following cases:

use of any of the amp-facebook(-*) tags is valid with inclusion of an amp-facebook-1.0.js script
use of any of the amp-facebook(-*) tags other than amp-facebook is invalid if only amp-facebook-0.1.js script is included (they must include their corresponding 0.1 script if not that amp-facebook-1.0.js script)
none of the amp-facebook-*-0.1.js extension scripts can be used on the same document as the amp-facebook-1.0.js script

Continuation from #35064 for the validator change discussions.

amp-owners-bot · 2021-07-26T15:24:52Z

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-facebook-comments/0.1/test/validator-amp-facebook-comments-invalid.html
extensions/amp-facebook-comments/0.1/test/validator-amp-facebook-comments-invalid.out
extensions/amp-facebook-comments/0.1/test/validator-amp-facebook-comments.html
extensions/amp-facebook-comments/0.1/test/validator-amp-facebook-comments.out
extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii
extensions/amp-facebook-like/0.1/test/validator-amp-facebook-like-invalid.html
extensions/amp-facebook-like/0.1/test/validator-amp-facebook-like-invalid.out
extensions/amp-facebook-like/validator-amp-facebook-like.protoascii
extensions/amp-facebook-page/0.1/test/validator-amp-facebook-page-invalid.html
extensions/amp-facebook-page/0.1/test/validator-amp-facebook-page-invalid.out
extensions/amp-facebook-page/validator-amp-facebook-page.protoascii
extensions/amp-facebook/1.0/test/validator-amp-facebook-comments.html
+10 more

Gregable · 2021-07-28T17:34:20Z

extensions/amp-facebook/1.0/test/validator-amp-facebook-comments.out

+|      layout="responsive"
+|      data-href="https://www.facebook.com/zuck/posts/10102593740125791">
+|    </amp-facebook>
+|    <!-- Example of a valid amp-facebook-comments -->


Do you intend for this to be valid? You may want to add the extension script in that case.

Same question for a few of the other tests below.

In this example, amp-facebook-comments, amp-facebook-like, and amp-facebook-page are all registered by the one amp-facebook script included. As long as amp-facebook-1.0 is being used, all four components should have valid use. Is that possible?

For reference, this is what I was having trouble expressing in this PR:

The script inclusion is not exclusive after applying satisfies/excludes, as can be seen with the passing cases that should be failing in extensions/amp-facebook/1.0/test/validator-amp-facebook-exclusive.html

Use of the amp-facebook-* components is not valid with the amp-facebook-1.0.js script alone -- it appears to be running into the requires_extension line, i.e.:

amphtml/extensions/amp-facebook-page/validator-amp-facebook-page.protoascii

Line 30 in 5869aed

requires_extension: "amp-facebook-page"

I assume there's not such a thing as requires_extension_oneof like we have with other properties? Example test file that should pass but fails: extensions/amp-facebook/1.0/test/validator-amp-facebook-page.out

I think I see now. I'm not 100% sure this'll work without running the tests, but can you define two amp-facebook-comments tagspecs, one requiring each extension but otherwise identical?

To reduce duplication a tiny bit, you can use a named attr_list with all of the attrs in it which you then include in both tagspecs.

Thanks for this recommendation! It looks like your recommendation, plus giving the 1.0 script a spec_name and requiring by that, did the trick for the second bullet point above (allowing amp-facebook-* to be used when importing amp-facebook:1.0). I added more tests to verify also that importing 0.1 does not work, but it helped me to notice that the secondary tag spec obscures the validation message for amp-facebook-*, which might normally recommend importing the script corresponding to the tag in use. Instead the invalid message shows:

The tag 'amp-facebook 1.0' is missing or incorrect, but required by 'amp-facebook-page'.

You'll need to give the different tagspecs spec_names now that there are more than one tag_spec for each of the tag names like amp-facebook-comments

Thanks for this recommendation! Adding spec_name now suffices for the other bullet point above (exclusion between tags).

As for the "obscured message", the more I think about it, the more I settle on the perspective that it is WAI. In that case things are all set for review. :) Thank you again for all your patience and wisdom here 🙏

extensions/amp-facebook/validator-amp-facebook.protoascii

Gregable

Suggestion for a few comments to toss in, but otherwise looks good.

Gregable · 2021-08-03T22:57:00Z

extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii

-    mandatory: true
-  }
+  requires: "amp-facebook-comments 0.1"
+  attr_lists: "amp-facebook"


I'd suggest leaving a comment for the reader on where to find this list, since it's in a different file.

Gregable · 2021-08-03T22:58:15Z

extensions/amp-facebook/validator-amp-facebook.protoascii

+  html_format: AMP
+  tag_name: "SCRIPT"
+  spec_name: "amp-facebook 1.0"
+  satisfies: "amp-facebook 1.0"


This would be a good place to leave a comment for the reader on what this scheme is trying to do, maybe even just linking to the discussion in this PR as sufficient. What do you think?

Good idea, linked this PR and a brief explanation.Thanks for the recommendation

westonruter · 2021-08-11T19:48:20Z

After looking at how the changes here relate to the AMP-WP plugin, I have a question about how this might impact extension auto-importing. Namely, if someone adds a <amp-facebook-comments> to the page, given that the 0.1 and 1.0 versions look identical, how should extension auto-importing (e.g. in the Optimizers) know which version to include? For one, the amp-facebook-comments 0.1 script is required but for the other amp-facebook 1.0 script is required. It seems that currently in the Node.js Optimizer will end up importing one version depending on which occurs last in the spec: https://github.com/ampproject/amp-toolbox/blob/c4093bfb167de112de03752c8e6abe07ab92ea1b/packages/optimizer/lib/transformers/AutoExtensionImporter.js#L92-L103

caroqliu · 2021-08-11T20:48:27Z

After looking at how the changes here relate to the AMP-WP plugin, I have a question about how this might impact extension auto-importing. Namely, if someone adds a <amp-facebook-comments> to the page, given that the 0.1 and 1.0 versions look identical, how should extension auto-importing (e.g. in the Optimizers) know which version to include? For one, the amp-facebook-comments 0.1 script is required but for the other amp-facebook 1.0 script is required. It seems that currently in the Node.js Optimizer will end up importing one version depending on which occurs last in the spec: https://github.com/ampproject/amp-toolbox/blob/c4093bfb167de112de03752c8e6abe07ab92ea1b/packages/optimizer/lib/transformers/AutoExtensionImporter.js#L92-L103

This is a good question. The amp-carousel versions 0.1 and 0.2 may set a useful example on what we should do here, or amp-next-page 0.1 and 1.0. Do you know whether the optimizer has accounted for these in the past? I suspect this may be one of the original use cases for us providing a -latest.js for components to use.

In terms of the qualitative difference between 0.1 and 1.0 for amp-facebook-comments in particular, the ideal behavior is likely subject to whether the document author intends to have a Bento document. I could imagine bento being a standalone option in the optimizer itself or an expansion of the autoExtensionImporter option.

Another consideration too is that the 1.0 version is still under experimental. Though the experiment is likely to be removed soon, in the immediate short term, I assume that would make 0.1 a safe default for the time being.

westonruter · 2021-08-11T21:06:21Z

I think the difference between these Facebook components and amp-carousel/amp-next-page is that not only is there a version difference there is also a script requirement difference. As far as I know, this is the first time this has been the case. So it seems that what auto-extensions importers will have to do is remove from consideration any versions of the components that they don't want. For example, in the AMP plugin we're going to have a Bento opt-in experiment and when this is active we'll need to make sure that we exclude this spec:

amphtml/extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii

Lines 29 to 45 in d21ecc0

    
           tags: {  # <amp-facebook-comments> 
        
             html_format: AMP 
        
             tag_name: "AMP-FACEBOOK-COMMENTS" 
        
             requires_extension: "amp-facebook-comments" 
        
             requires: "amp-facebook-comments 0.1" 
        
             # The amp-facebook attr_list is defined in the amp-facebook protoascii 
        
             attr_lists: "amp-facebook" 
        
             attr_lists: "extended-amp-global" 
        
             amp_layout: { 
        
               supported_layouts: FILL 
        
               supported_layouts: FIXED 
        
               supported_layouts: FIXED_HEIGHT 
        
               supported_layouts: FLEX_ITEM 
        
               supported_layouts: NODISPLAY 
        
               supported_layouts: RESPONSIVE 
        
             } 
        
           }

And only consider this one:

amphtml/extensions/amp-facebook/validator-amp-facebook.protoascii

Lines 66 to 82 in d21ecc0

    
           tags: {  # <amp-facebook-comments> via amp-facebook script 
        
             html_format: AMP 
        
             spec_name: "amp-facebook-comments 1.0" 
        
             tag_name: "AMP-FACEBOOK-COMMENTS" 
        
             requires_extension: "amp-facebook" 
        
             requires: "amp-facebook 1.0" 
        
             attr_lists: "amp-facebook" 
        
             attr_lists: "extended-amp-global" 
        
             amp_layout: { 
        
               supported_layouts: FILL 
        
               supported_layouts: FIXED 
        
               supported_layouts: FIXED_HEIGHT 
        
               supported_layouts: FLEX_ITEM 
        
               supported_layouts: NODISPLAY 
        
               supported_layouts: RESPONSIVE 
        
             } 
        
           }

The metadata to do so seems a bit fuzzy however.

westonruter · 2021-08-11T21:13:38Z

In short, we'll have to merge bundles.config.extensions.json with the validator spec data. We're doing that now to a degree, but we'll have to go further.

honeybadgerdontcare · 2021-08-11T21:27:07Z

You're saying there needs to be more metadata to assist the Optimizer in doing this, right? Whether that be having another file that contains just the known metadata (e.g. when bento don't use this spec_name, use this one instead) or add a metadata identifier within the validator.proto to be used in the rules just for bento and for the optimizer (e.g. is_bento: true, defaults to false).

westonruter · 2021-08-12T04:29:34Z

Right. Basically, when there are two different tag specs that look the same except they have differences in the required_extension, where one requires a Bento extension and another requires a non-Bento extension, this is where we need to tell the difference between the two.

I was able to work out a way to do so by using the requires and satisfies to link them together along with data from bundles.config.extensions.json (in not a terribly elegant way, but it works).

It seems it would be useful if there was an is_bento: true field added to the extension_spec:

amphtml/extensions/amp-facebook/validator-amp-facebook.protoascii

Lines 18 to 35 in d21ecc0

    
           tags: {  # amp-facebook 1.0 
        
             html_format: AMP 
        
             tag_name: "SCRIPT" 
        
             # The 1.0 amp-facebook script registers all amp-facebook-* components. 
        
             # See https://github.com/ampproject/amphtml/pull/35395 for context. 
        
             spec_name: "amp-facebook 1.0" 
        
             satisfies: "amp-facebook 1.0" 
        
             excludes: "amp-facebook 0.1" 
        
             excludes: "amp-facebook-comments 0.1" 
        
             excludes: "amp-facebook-like 0.1" 
        
             excludes: "amp-facebook-page 0.1" 
        
             extension_spec: { 
        
               name: "amp-facebook" 
        
               version_name: "v1.0" 
        
               version: "1.0" 
        
             } 
        
             attr_lists: "common-extension-attrs" 
        
           }

This would prevent us from having to consult bundles.config.extensions.json, but then it would mean there'd be some duplication in the information across these two sources.

Attempt adding validations

10f64aa

caroqliu requested a review from honeybadgerdontcare July 26, 2021 15:24

twifkak added the WG: caching label Jul 27, 2021

Gregable requested changes Jul 28, 2021

View reviewed changes

caroqliu added 2 commits July 29, 2021 15:00

Invert satisfies/excludes for amp-facebook:0.1

9fb3bad

Merge branch 'main' into validate-facebook

97ff535

caroqliu requested a review from Gregable July 30, 2021 13:39

caroqliu added 7 commits July 30, 2021 13:14

Include additional tagspecs

db6a4d7

Incorrectly passing cases

bfa95b2

Require by spec_name

adc7255

Close amp-facebook-comments 0.1 tag

311907c

Make 0.1 and 1.0s exclusive

96739d0

Give spec_names to amp-facebook-page tag

fee97ea

Clean up recent changes

84196e1

caroqliu requested a review from alanorozco August 2, 2021 21:37

Give unique spec_name

1b4c1aa

Gregable approved these changes Aug 3, 2021

View reviewed changes

Include comments

90f8a44

amp-bundle-size bot requested a review from samouri August 4, 2021 14:03

caroqliu added 2 commits August 4, 2021 10:21

Remove trailing whitespace

966b4d7

Merge branch 'main' into validate-facebook

828cf87

caroqliu merged commit b087dbd into ampproject:main Aug 4, 2021

caroqliu mentioned this pull request Aug 9, 2021

I2I: Consolidated 1.0 amp-facebook #34969

Closed

ampprojectbot added the PR use: In Beta / Experimental label Aug 11, 2021

westonruter mentioned this pull request Aug 11, 2021

Update amphtml spec to dc6cd22a52 ampproject/amp-wp#6530

Merged

6 tasks

ampprojectbot added the PR use: In Stable label Aug 17, 2021

honeybadgerdontcare mentioned this pull request Aug 20, 2021

Indicate bento supported versions within ExtensionSpec #35756

Closed

westonruter added the WG: bento label Sep 5, 2021

ampprojectbot added the PR use: In LTS label Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bento: Validate Facebook components #35395

Bento: Validate Facebook components #35395

caroqliu commented Jul 26, 2021 •

edited

Loading

amp-owners-bot bot commented Jul 26, 2021 •

edited by ampprojectbot

Loading

Gregable Jul 28, 2021

caroqliu Jul 29, 2021

caroqliu Jul 29, 2021

Gregable Jul 30, 2021 •

edited

Loading

caroqliu Jul 30, 2021

Gregable Jul 30, 2021

caroqliu Aug 2, 2021

Gregable left a comment

Gregable Aug 3, 2021

caroqliu Aug 4, 2021

Gregable Aug 3, 2021

caroqliu Aug 4, 2021

westonruter commented Aug 11, 2021

caroqliu commented Aug 11, 2021

westonruter commented Aug 11, 2021 •

edited

Loading

westonruter commented Aug 11, 2021

honeybadgerdontcare commented Aug 11, 2021

westonruter commented Aug 12, 2021

Bento: Validate Facebook components #35395

Bento: Validate Facebook components #35395

Conversation

caroqliu commented Jul 26, 2021 • edited Loading

amp-owners-bot bot commented Jul 26, 2021 • edited by ampprojectbot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gregable Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gregable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Aug 11, 2021

caroqliu commented Aug 11, 2021

westonruter commented Aug 11, 2021 • edited Loading

westonruter commented Aug 11, 2021

honeybadgerdontcare commented Aug 11, 2021

westonruter commented Aug 12, 2021

caroqliu commented Jul 26, 2021 •

edited

Loading

amp-owners-bot bot commented Jul 26, 2021 •

edited by ampprojectbot

Loading

Gregable Jul 30, 2021 •

edited

Loading

westonruter commented Aug 11, 2021 •

edited

Loading