AMP package

🗣 Feature Discussions Currently in Development
28

You are right, the frontity.render does hold the default render method so one can do just that, easily.

The only benefit of defaultRenderer method is that the package author would not have to ensure the passing of the ...rest arguments since the serializer has already been defined. That translates to something in the line of, if the user has no entry points, the defaultRenderer will call renderToStaticMarkup instead and not collect the chunks, and so on.

How I see it, render should be used only when the functionality needs to do something after the App has been serialized to a string. If the functionality only needs to wrap the App with providers/custom elements, the App component should be overwritten instead and let frontity handle the render.

Do you see the closure capture pattern as a better one? I have no preferences or strong opinions really and my guess is that this will apply to template as well.

Actually I think the closure capture pattern will enforce a model of always having to call the previous methods, so it’s gonna be an imperative call and the expectancy will be set accordingly.

:+1: thanks for the feedback @luisherranz :smiley: gonna update the implementation and not send the default<method> at all.

29

Interesting :slightly_smiling_face:

To be honest, my plan was to:

  1. Expose the different parts and let packages overwrite/wrap them, depending on the case.
  2. Add priorities. Because if a package needs to overwrite, like AMP, it needs to be executed first or it will erase the wrappers.

But maybe we can have a call next week to discuss the different use cases and how each approach would work in each case :+1:

30

Got some more updates and I’ve managed to achieve the closure capture pattern seamlessly! :slightly_smiling_face: Really comfortable now that the packages are forced now to use this pattern, that will not lead to inconsistencies to the output.

The final package implementation for AMP would looks like this:

import { CacheProvider } from "@emotion/react";
import createEmotionServer from "@emotion/server/create-instance";
import createCache from "@emotion/cache";

export default {
  // [...] rest of the .settings.js
  actions: {
    theme: {
      beforeSSR({ libraries: { frontity } }) {

        // Get the previous reference
        const previousRender = frontity.render;

        // Let's say you want to 'custom' render your app with special tags.
        frontity.render = ({ App, ...rest }) => {
          const key = "frontity";
          const cache = createCache({ key });
          const { extractCritical } = createEmotionServer(cache);
          
          // Call the extractCritical. This will return an object as { html, ids, css }
          const result = extractCritical(
            // `previousRender` is the default render method that Frontity uses internally.
            previousRender({
              App: () => (
                <CacheProvider value={cache}>
                  <App />
                </CacheProvider>
              )
            })
          );
          
          // We can safely return here the emotion critical call result
          // since we're gonna handle this result ourselves.
          return {
            ...result,
            key
          };
        };
        
        const previousTemplate = frontity.template;        

        // Custom `template` function.
        frontity.template = ({ result, head, ...rest }) => {
          // We grab the resulted html, ids, css and the cache key.
          const { html, ids, css, key } = result;
          
          // And we push to the `head` the new style tag.
          head.push(
            `<style data-emotion="${key} ${ids.join(' ')}">${css}</style>`
          );
          
          // And we then pass along the rest of the arguments
          // with the new `head` and the `html`.
          return previousTemplate({
            ...rest,
            head,
            html
          });
        };
      }
    },
  },
};
31

Awesome :slightly_smiling_face:

There is one thing I didn’t include in the IP but maybe it makes sense: Expose the inners of the template in libraries.

  • libraries.frontity.head.title
  • libraries.frontity.head.meta
  • libraries.frontity.head.script
  • libraries.frontity.head.style
  • …

Then, when we get the ones generated in React, we either overwrite them or merge them:

const head = getHeadTags(helmetContext.helmet);

// Maybe overwrite some values.
libraries.frontity.head.title = head.title;

// And merge others.
libraries.frontity.head.meta += head.meta;
libraries.frontity.head.script += head.script;
libraries.frontity.head.style += head.style;

I wonder if we can use this to hook from the AMP into the head.style and refactor it in a way that all the styles end up inside the single <style amp-custom> tag.

32

I’ve just realized that there is another requirement for AMP that I forgot in the initial proposal.

Make AMP pages discoverable

AMP pages need a <link rel="amphtml"> element, like this:

<link rel="amphtml" href="https://domain.com/some-post/amp/" />

And HTML pages need a <link rel="canonical"> element, like this:

<link rel="canonical" href="https://domain.com/some-post/" />

We don’t need this for the beta because it can be added manually to the theme, but we need to think a bit about how to solve it.

My first thought is that maybe we need to add the @frontity/amp package to the main site as well, but using the src/index.js entry point.

It will need information about:

  • How URLs are built:
    • /amp
    • amp.domain.com
    • ?amp=true
    • Something custom.
  • What URLs have AMP versions:
    • All pages, including archives.
    • All post types, including pages and cpts.
    • Only posts.
const settings = [
  {
    name: "main-site",
    packages: [
      {
        name: "@frontity/amp",
        state: {
          amp: {
            site: "main",
            urls: "query",
            types: "posts",
          },
        },
      },
    ],
  },
  {
    name: "amp-site",
    match: "(\\?|&)amp=",
    packages: [
      {
        name: "@frontity/amp",
        state: {
          amp: {
            site: "amp",
            urls: "query",
            types: "posts",
          },
        },
      },
    ],
  },
];

Feedback is welcomed :slightly_smiling_face:

33

I have created this GitHub board to track the status of the different issues related to this AMP feature. Some comments about it:

  • I feel it could be useful for people wanting to know the status of each of them or the whole feature.
  • If someone wants to help it could be helpful as well, as they could ask to develop any of the issues that aren’t In progress as explained in this workflow.
  • I created also an amp-beta label to track the issues that are needed to release the beta version of the @frontity/amp package.
  • I added a description to each issue with the relevant information written in this thread.
  • It should be automated thanks to GitHub.

I’ve just realized that there is another requirement for AMP that I forgot in the initial proposal.

I’ve added it to the GitHub board as well :slightly_smiling_face:.

1 Like
34

I have been thinking a little more about this and another option could be to use a different package, for example @frontity/amp-tags.

The options could be:

interface AmpTags {
  state: {
    amp: {
      /* @defaults "path" */
      type: "path" | "subdomain" | "query";

      /* @defaults "amp" */
      value: string;

      /* @defaults "posts" */
      entities: "all" | "postTypes" | "posts";
    };
  };
}

I’m not convinced on the property names (type, value, entities) but I hope they are enough for the example.

A default @frontity/amp-tags would add amphtml links to the same URLs with ending /amp, and only for “posts”, which is the most common use case in WordPress.

const settings = [
  {
    name: "main-site",
    match: "...",
    packages: [
      "@frontity/amp-tags",
      //...
    ],
  },
];

If people want to use a subdomain or a query, they need to add some settings:

// For amp.mydomain.com
state: {
  amp: {
    type: "subdomain",
  },
},
// For other.mydomain.com, all post types
state: {
  amp: {
    type: "subdomain",
    value: "other",
    entities: "postTypes",
  },
},
// For ?amp=true
state: {
  amp: {
    type: "query",
  },
},
// For ?other=true, all URLs
state: {
  amp: {
    type: "query",
    value: "other",
    entities: "all",
  },
},

And so on…

I think making this in an independent package would make sense and it will be simpler.

35

Hey @luisherranz I am not sure that I understand the need for a separate package for handling the tags. Do you think that this is not gonna be something that folks would always have to do? So having it in a separate package for those who need it makes sense?

36

Well… doing this in a separate package is not 100% required, but it may simplify things in our end and it requires less configuration in the frontity.settings.js file.

Yes, the non-AMP site needs the tags, or Google (and other services) won’t be able to discover the AMP version of the pages.

37

Oh, totally get it now! :smiley: thank you! Yeah it makes sense then.

38

Hey @SantosGuillamot I’d like to agree on what the minimum required processors should be exactly :slight_smile:

I’m looking at the the old framework and also looking at the AMP HTML Specification to figure out what is necessary.

So, looks like there are 3 general types of processors that we’re gonna need:

1. Replacing the built-in components with AMP components:

  • <audio> → <amp-audio>
  • <img> → <amp-img>
  • <video> → <amp-video>
  • <iframe> → <amp-iframe>

2. Processors that remove elements & attributes prohibited in AMP or that don’t make sense

  • Remove the style tags
  • Remove script tags
  • Remove any id, class or attribute names that start with -amp- and i-amp-. They are prohibited in AMP.
  • Remove any elements that have hidden or style="display:none" (external JS is prohibited so those are always going to stay hidden)
  • Remove frame, frameset, object, applet, param & embed elements. I think those are not really used in wordpress or on the web in general, so perhaps it’s not even necessary. I don’t see those in the old version of the framework.

3. Processors for social embeds

  • instagram
  • twitter
  • facebook
  • soundcloud
  • youtube
  • etc.

There are many other embeds of course that have a dedicated AMP component like embed for Vimeo, etc. The ones I mention here are the ones that were present in the old version of the framework.

I think that those are all the necessary ones.

I also see a couple of processors in the old version of the framework that I think will not be necessary anymore:

  • Remove body tag
  • Remove “content ads”
  • Remove doctype tag
  • Remove head tag
  • Remove html tag

Finally, there are processors in the old version of the framework that I simply don’t understand the purpose of (so they might be necessary or might not):

@luisherranz Please correct me if I’m wrong on those last two points :slight_smile:

39

I’ve also realized that there is a small hurdle with the the AMP processors:

Most AMP components require the user to load an external script. For example the <amp-iframe> requires the user to add the following script:

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

How was this handled in the “old” version of the framework? I think that we could add something like the following to each processor that will need to load a script, but I’m not sure if there isn’t a better solution.

export const iframe: Processor<Element, Packages> = {
  // Find an element that has an iframe as a child
  test: ({ node }) =>
    node.children.some(
      (child) => child.type === "element" && child.component === "iframe"
    ),
  processor: ({ node }) => {
    // Define a <script /> element equivalent to the required AMP script
    const script: Element = {
      type: "element",
      component: "script",
      props: {
        async: true,
        "custom-element": "amp-iframe",
        src: "https://cdn.ampproject.org/v0/amp-iframe-0.1.js",
      } as any,
    };

    // Add the script as the first child and then render the rest of the children
    node.children = [script, ...node.children];
    return node;
  },
};
40

Hey Michal, great summary :grinning_face_with_smiling_eyes:

It was handled in React, injecting the <script> with <Head> (Helmet back then):

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 didn’t work on those. @David, do you have any memories about that?

41

Thanks for the great summary @mmczaplinski :slightly_smiling_face:

Just for the record: Our idea is to release an initial list of processors and once the beta version of the package is ready, run the AMP-validator in our web and with some partners to see what could be missing. With this, most of the use cases should be covered and from there, if any users find something new, they can suggest a new processor or even do a Pull Request for it.

Having said that, and according to your comments, this should be the way to proceed I guess:

Initial list of processors

  • Audio
  • Video
  • Image
  • Iframe
  • Remove style tags
  • Remove script tags
  • Remove any id , class or attribute names that start with -amp- and i-amp- .
  • Remove any elements that have hidden or style="display:none"
  • Instagram
  • Twitter
  • Facebook
  • Soundcloud
  • Youtube

I think once we agree on this list, it could be nice to add them in the Pull Request as tasks too.

Out of the list

We will see after validating AMP if some of them are needed or not.

  • Remove frame , frameset , object , applet , param & embed elements: If they are not used I wouldn’t include them at first and we’ll see if there are really needed after the validation.
  • I guess these ones are not needed because we are overwriting the HTML?
    • Removebody tag?
    • Remove head tag?
    • Remove html tag?
    • Remove doctype tag?
  • Remove “content ads”: I am not sure but maybe this one wasn’t part of the amp version but just part of the old framework. Anyway, I guess that if something is needed it should be handled with the ads packages.
  • 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.
42

These ones were created because some of the Frontity PRO clients had full HTML documents inside their post content. I guess it was due to some weird copy-paste of their content editors from another medium.

I don’t think they are needed anymore.

43

okay, thanks for your comments guys! :slight_smile:

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 :+1:

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

44

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 :man_facepalming:

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.

45

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.

46

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.

47

Hey folks, I’ve started yesterday to work on Make @frontity/comscore-analytics AMP-aware [5pt] #712 and I have some questions that I think is worth mentioning here.

Regarding “AMP-aware components”, this is what is written down in the implementation proposal:

My questions are: Does the amp entry point still apply? Or this is how we should code this kind of components? Could we create two different components for each mode or is that not possible anymore?