Amphtml: Discussion: Require a matching amp-* tag when including new amp extension javascript

[Gregable]

Currently, if an AMP page makes use of an amp extension (eg <amp-foo>) the AMP validator requires that the matching script tag be located on the page, for example:

<script async custom-element='amp-foo' src='https://cdn.ampproject.org/v0/amp-foo-0.1.js'></script>

However, the reverse is not true. You can include amp-foo-0.1.js without the presence of any <amp-foo> tag and it is not a validation error.

This results in an anti-pattern of folks generating a header for a site that includes the superset of all javascript extensions that they may want to use, even if a given page does not use that extension. The AMP cache can strip unused extensions, but the origin is still affected.

See Design Principles: "User Experience > Developer Experience > Ease of Implementation."

When in doubt, do what’s best for the end user experience, even if it means that it’s harder for the page creator to build or for the library developer to implement.

I'd like to propose that for new extensions, we enforce both requirements. If amp-new-extension-0.1.js is on the page but <amp-new-extension> isn't, for example, that would be a validator error. Existing extensions would be grandfathered in, at least for now.

The big risk I see is that a developer using the superset header might add one of these new extensions to their header and accidentally invalidate their entire site without realizing it.

[erwinmombay]

what about something like amp-access which is not a custom-element?
nevermind missed the grandfathering old components part

[cramforce]

I hadn't thought about the fact that we can do this in a backward compatible manner. I like that a lot. This is one of the optimizations that the cache can do transparently, but it is definitely better to have the sources be correct to avoid weird side effects.

We do and will have a few "headless" extensions that do not need a script, but those can be special cased.

LGTM from me.

[Gregable]

We do and will have a few "headless" extensions that do not need a script, but those can be special cased.

Of course.

[mkhatib]

One exception in the naming currently is amp-form script doesn't have an <amp-form> element and is only an extension. I believe amp-dynamic-css-classes are the same thing.

[Gregable]

I'm going to mark this as closed and make this our policy going forward for new amp extensions. We'll watch out for the specific oddball cases such as amp-form requiring <form> (though this example is grandfathered in).

[ericlindley-g]

Reopening with a complication: currently it's possible to create a page using amp-live-list that doesn't use an extension at time 0, but then uses that extension at time 1 (because an update to the page uses amp-youtube, for instance), and then does not use that extension at time 2. Our recommendation (until we figure out a better fix) is for developers to include all extensions needed for all items in amp-live-list in the page, regardless on whether they're in the page at all times.

If the change in this issue results in an error, then pages as described above will switch from valid to invalid, as new content comes in or is removed from the page.

Are we emitting a warning or an error right now? Can we just do a warning until the amp-live-list issue has a good solution?

[Gregable]

I don't think that's possible with <amp-live-list>. I think the current implementation does not allow introducing new tags, only changing attributes on the tags (and making multiple copies of them). The contents of this tag are a template, already included on the page.

[ericlindley-g]

@Gregable That's right—there's no way to pass missing tags into the main doc with new entries in amp-live-list at this time, so (if I understand correctly):

• Given a header template that enables only the core components and say, amp-youtube
• Assuming that we release a new component amp-cats, and apply the rules described in this issue.

Then, given the following sequence:

1. a live blog has an initial series of posts that don't contain amp-cats
2. the live blog posts an entry with amp-cats
3. eventually, enough new entries are posted that the entry in (2) is pushed off the page, and the page no longer contains any posts with amp-cats

If the script for amp-cats is included on the page, the page at steps (1) and (3) are invalidated by the change proposed in this issue
If the script for amp-cats is not included on the page, the page at step (2) is invalidated by the change proposed in this issue

Is that correct? If so, should we issue a warning instead of an error for including a script without the corresponding component in the page — at least until amp-live-list has a solution for this issue? Or should we lift this restriction for only those pages that contain amp-live-list?

[Gregable]

In this scenario, the publisher could remove the extension when it's no longer used. They are generating the document on each version, so they could detect and remove the extension.

This is not an issue for the page that has already loaded amp-cats, as validation is done once and we don't require validity for the combined original document along with the new content injected into it

[ericlindley-g]

That's true: the publisher could remove and re-insert the extension, but it adds some extra logic, work, and risk that a pub won't be aware of this constraint. We've recommended the approach in the other bug just to load all possible extensions from the start—and though we can change that recommendation based on what we think is best for users, I'm hesitant to do it, given the complexity of a publisher-side fix. (I'd be curious to hear @cramforce 's thoughts here)

Also true that validation for the page itself doesn't fail for the page that has already loaded amp-cats, but if I'm following the chain of events correctly, I think the host document that the client is pulling from would get invalidated if amp-cats is removed from it, and so any new items while that host document is invalidated won't come into the client. (lots of edge and corner cases on this one... : P

[Gregable]

Sorry, I was confusing <amp-live-list> with <amp-list>. With <amp-live-list>, if a post uses a new amp tag, then there is no way to communicate that to the parent document in the general case. Now I understand.

Few documents will likely ever hit this corner case, since I would imagine the custom elements used in one list element are likely to be the same set in later ones. It doesn't seem like our current recommendation of just loading all possible elements in advance is all that great for the corner case though. It still assumes that the author knows in advance the set of amp extensions they might need (or it leads to loading a huge list which is a performance issue). It also doesn't address what to do if the new post comes along with new style rules, for example.

Maybe we should address this by trying to come up with a different solution to loading head content, such as having the runtime load the necessary extensions by inspecting the tags on the page. Runtime can assume that the content is valid, so if it spots <amp-cats>, it should be safe to assume that amp-cats-latest.js exists and simply load it?

[ericlindley-g]

Definitely would be curious to find a way to fix this directly. @Gregable, do you know which extensions are affected by this change, now (none released before Oct 11, I assume, but is there an easy way to list?)

[Gregable]

So far, only amp-app-banner is affected.

[ericlindley-g]

Excellent—thanks! Essentially no-risk then, so we have time for the actual solution

[maxhartshorn]

amp-playbuzz is also affected

[maxhartshorn]

@Gregable @ericlindley-g with respect to corner/edge cases I could see them popping up quite a bit. In our organization for example producers regularly embed all manner of different content into live-blog posts so it would be very likely new embed types would appear on a live post that were not present on load.

[ericlindley-g]

@Gregable , are we issuing warnings or errors for new components at this point? (that is, are we triggering a validation for amp-playbuzz and amp-app-banner at this point, or just a warning, like with the other components?)

[jasti]

@Gregable I'd like to reopen this based on some client feedback. Had a discussion with @powdercloud and @dvoytenko and we converged on making unused extensions emit a warning instead of an error (which happens for a select set of extensions).
Right now, the errors are flagging publisher docs as invalid and therefore don't appear on Search.
As @ericlindley-g mentioned above - it's not reasonable to expect every publisher to apply this logic whenever pushing updates to page content -especially if they are using 3P CMSs.

[dvoytenko]

My thoughts on this:

1. The amp-live-list case where the new update can require new extensions should be supported directly and I would consider it a bug: the new updates should automatically install newly required extensions. This is typically also a rather easy fix. Please file an issue if this is the case.
2. I don't see how we can require the unused scripts to be cleaned up in most of cases at this time. The tools that are typically used today are simply unable to do this. So I'm very hesitant to force this as an error. I'm very happy with a warning however. I also have concerns that we may start forcing some workarounds to such rules that would make the situation a lot worse.

[dvoytenko]

To add to this: the point (2) is about "most of extensions". There could be some extensions that we maybe more strict about - those that change the nature of the page. But that's an exception, not the rule.

[ampprojectbot]

This issue hasn't been updated in awhile. @Gregable Do you have any updates?

[Gregable]

No. I think the current state is reasonable. We have a separate issue to track the amp-live-list changes. assigning to @dvoytenko to determine if anything more needs to happen here and close if not.

[Gregable]

As per no response, closing.

Of note, I think we're also going to remove the warnings for the grandfathered unused extensions, but leave the errors for all new unused extensions.