AMP package

[mmczaplinski]

okay, thanks for your comments guys!

@luisherranz

What I don’t remember is how we avoided the injection of multiple <script> tags when multiple AMP components are used.

Maybe react-helmet is smart enough to figure out duplicates and it removes them because I don’t see any special code to prevent that.

I think that react-helmet should handle the duplicates just fine. As far as I can tell, it should override the previous tag if we add another one with the same type and props. And even if the DOM is updated with a new script tag that should not trigger loading the script as far as I understand. I will test it though

@SantosGuillamot

• Processor to remove AMP list types: According to this issue, the type attribute wasn’t supported back then but it seems supported now so it shouldn’t be needed.
• Processor to remove AMP column widths: I am not sure about this one. But according to this other issue, there was a problem with the width in <col> tags, so I guess that’s why we created a processor. I don’t know how this should be handled now.

Thanks for digging up the issues @SantosGuillamot That clears things up

[mmczaplinski]

@mmczaplinski

And even if the DOM is updated with a new script tag that should not trigger loading the script as far as I understand. I will test it though

Whoops, that made 0 sense because we don’t run stuff on the client in AMP of course so we’re not updating the DOM

I’ve tested it and indeed react-helmet-async will just spit out 1 script tag on the server, even if we add call <Helmet> multiple times - for example for each iframe element in the page.

[mmczaplinski]

I notice that in some of the AMP processors in the previous version of the framework, we did some additional work like ensuring that iframes load over HTTPS or calculating the dimensions of the <video/> element.

I think the burden should be on the user to ensure that their content conforms to the AMP spec. Both because it could make the user think that their content is AMP-compliant when it’s not and also because there are probably too many restrictions on the AMP content for us to reasonably be able to catch all of them.

In case that we decide to still implement this, I think it should be beyond the scope of the beta version of the AMP package.

[luisherranz]

Another idea for the amphtml tags in the main site could be to add them with an option on the @frontity/yoast and @frontity/head-tags packages.