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. 
Ok, Iāve recorded a loom explaining my reasoning
Let me know what you think.
Great explanation Cristian, thanks 
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.comsubdomain. - An
?amp=truequery. - An ending
/amppath.
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
matchregexp.
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.
thanks for the feedback @luisherranz 
gonna update the implementation and not send the default<method> at all.
Interesting
To be honest, my plan was to:
- Expose the different parts and let packages overwrite/wrap them, depending on the case.
- 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
Got some more updates and Iāve managed to achieve the closure capture pattern seamlessly! 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
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.titlelibraries.frontity.head.metalibraries.frontity.head.scriptlibraries.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:
/ampamp.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
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 progressas explained in this workflow. - I created also an
amp-betalabel to track the issues that are needed to release the beta version of the@frontity/amppackage. - 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 .
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! thank you! Yeah it makes sense then.
Hey @SantosGuillamot Iād like to agree on what the minimum required processors should be exactly 
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
styletags - Remove
scripttags - Remove any
id,classor attribute names that start with-amp-andi-amp-. They are prohibited in AMP. - Remove any elements that have
hiddenorstyle="display:none"(external JS is prohibited so those are always going to stay hidden) - Remove
frame,frameset,object,applet,param&embedelements. 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
- 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
bodytag - Remove ācontent adsā
- Remove
doctypetag - Remove
headtag - Remove
htmltag
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
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 
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
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,classor attribute names that start with-amp-andi-amp-. - Remove any elements that have
hiddenorstyle="display:none" - 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&embedelements: 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?
- Remove
bodytag? - Remove
headtag? - Remove
htmltag? - Remove
doctypetag?
- Remove
- 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.