AMP package

That would be really useful indeed. Not particular for emotion in our case, but I can see this easily be the case to push 1st or 3rd party scripts with ease.

Alrighty, so this is achievable but only on the packages side. As it is proposed and implemented right now, Frontity only reads the properties defined on .frontity namespace. That means effectively one package can override previously defined ones. This is not a drawback in my opinion as these three properties are quite flexible and one needs to be careful about them.

In the case of multiple .render methods, each package should check of a previous value of frontity.render and depending on what exactly it’s needed to be achieved should call the previously defined method. Example:

export default {
  name: "baz",
  actions: {
    baz: {
      beforeSSR({ libraries: { frontity } }) {
        // Hold the previous render in a reference
        const previousRender = frontity.render;

        // Define the new method
        frontity.render = ({ App, defaultRenderer, ...rest }) => {
          // In this case we want to wrap the App with a `<baz>` element
          // but we could imagine this a provider as well,
          // but in that case `.App` should be used instead.
          let BazApp = () => (
            <baz>
              <App />
            </baz>
          );

          // If there's a previousRender defined pass along the new BazApp
          if (previousRender) {
            return previousRender({
              App: BazApp,
              defaultRenderer,
              ...rest,
            });
          }

          // If not use the `defaultRenderer`
          return defaultRenderer(<BazApp />);
        };
      },
    },
  },
};

Another option is that we could change the .render API surface to instead be a queue. So that would look something like this.

frontity.setRender(({ App, defaultRenderer }) => {
  return defaultRenderer(<App />);
});

But this will not be more effective in handling multiple .render definitions.

With the .render method we allow total control of the rendering to a package. And because that means we can not determine how a package will handle it’s own rendering – either using defaultRenderer or it’s own renderer – I don’t believe there’s an API surface that will allow multiple render definitions in a sane way.

But I wanna explore something before settling this. :sweat_smile:

Ok, I’ve recorded a loom explaining my reasoning

Let me know what you think.

Great explanation Cristian, thanks :slightly_smiling_face:

Yes, I also think we could go with the wrapper pattern for now and see what happens. We still need to add priorities to the packages, so those who need to go first or last can define it.

I have one question though: What is the benefit of a defaultRender variable? If the default render is populated in libraries.frontity.render before we run the middleware/actions, wouldn’t that be the same?

Something like this:

export default {
  name: "baz",
  actions: {
    baz: {
      beforeSSR({ libraries: { frontity } }) {
        // libraries.frontity.render already points to the default renderer.
        const previousRender = frontity.render;

        frontity.render = ({ App, ...rest }) => {
          let BazApp = () => (
            <baz>
              <App />
            </baz>
          );

          return previousRender({
            App: BazApp,
            ...rest,
          });
        };
      },
    },
  },
};

I’ve just realized that differentiating between AMP/not-AMP paths in the code that removes /amp from the links (this issue) is not needed at all.

The way the @frontity/amp package is going to work is with a new site:

export default [
  {
    name: "normal-site",
    packages: [
      // Normal packages.
    ],
  },
  {
    name: "amp-site",
    match: "\\/amp\\/?($|\\?|#)"
    packages: [
      "@frontity/amp",
      // Normal packages.
    ],
  },
];

It is going to be done that way because an optimized site should not have additional AMP code added to its bundle. With different sites, we can create independent bundles.

The way to load the AMP site is with a match. The usual configurations are:

  • An amp.domain.com subdomain.
  • An ?amp=true query.
  • An ending /amp path.

The /amp path will always load the AMP site, so things like /category/amp will always match.

This means that the /amp option doesn’t support using amp for a term or post slug. When people need that, they have to either:

  • Choose another option (subdomain or query).
  • Add it manually to the match regexp.

We should add a warning about this limitation in the documentation when explaining the /amp option.

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.

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:

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
          });
        };
      }
    },
  },
};

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.

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:

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

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.

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?

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.

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

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:

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;
  },
};

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?

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.

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.