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.

[David]

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:

@luisherranz

AMP-aware components

The components that we export in @frontity/components need to be aware of an AMP render and act accordingly.

For example, the Image component needs to return <amp-img> instead of <img> and so on.

const Image = ({ state, src }) => {
  if (state.amp) {
    return <amp-img src={src} />;
  } else {
    return <img src={src} />;
  }
};

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?

[luisherranz]

@David

Does the amp entry point still apply?

Do you mean having both a src/index.js for React and a src/amp.js for AMP?

[luisherranz]

By the way, I was thinking that maybe doing the AMP-aware logic based on state.amp is not safe enough.

If a user uses by mistake some AMP settings in his main site it will break the site. For example:

const settings = {
  name: "main",
  state: {
    frontity: {
      url: "https://domain.com",
    },
    amp: {
      ssrFallback: true // <- Breaks the site because `state.amp` is truthy
    }
  },
  packages: [
    // ...
  ],
}

What about state.amp?.isAmp:

const Image = ({ state, src }) => {
  if (state.amp?.isAmp) {
    return <amp-img src={src} />;
  } else {
    return <img src={src} />;
  }
};

We could also use a mode setting in the frontity namespace:

const Image = ({ state, src }) => {
  if (state.frontity.mode === "amp") {
    return <amp-img src={src} />;
  } else {
    return <img src={src} />;
  }
};

Or just populate isAmp in the frontity namespace:

const Image = ({ state, src }) => {
  if (state.frontity.isAmp) {
      return <amp-img src={src} />;
        } else {
            return <img src={src} />;
              }
              };
              ```

[David]

@luisherranz

Do you mean having both a src/index.js for React and a src/amp.js for AMP?

Yes, I mean exactly that.

I already implemented the changes using a src/amp.js file for the AMP version of the component but, after reading again the implementation proposal, I think that’s not the way of doing it.

Also, reading the docs is like there are only two entry points: client.js and server.js (apart from index.js). In the case of AMP components, what would be the way of separate the AMP logic from the non-AMP logic? Or are developers forced to ship the same code for AMP and non-AMP components?

Well, we could still create two different components, one for client.js and another one for server.js, and add the AMP logic only to the server.js's component. Is this the expected approach?

[luisherranz]

The “entry points setting” is part of the AMP IP, but it is not in the tasks for the beta.

So for now add the logic to the src/server.js file and once we have that feature we will move it to the src/amp.js file if necessary.

[David]

@luisherranz

So for now add the logic to the src/server.js file and once we have that feature we will move it to the src/amp.js file if necessary.

Thanks @luisherranz for the clarification, I’ll do that.

[luisherranz]

No problem

[cristianbote]

In the case of AMP components, what would be the way of separate the AMP logic from the non-AMP logic? Or are developers forced to ship the same code for AMP and non-AMP components?

Someone who wants to have a amp website, will have to define a different set of settings, for that website. So, the codebase will be the same, but the processors and delivery will be different. Or is it allowed to include custom amp components as such the main website has different content than the amp one? AMP noob here

I’ve always imagined that if one wants to have an AMP aware website, by using our amp package, amp processors, and such, including custom components should not be something that would be possible?

[David]

Well, not custom components but a different implementation of the same component. If I remember correctly, in the past we did that for complex components like carousel galleries and such that needed a different implementation for AMP.

And, I guess it would be better if you don’t add the AMP code to the bundle as it’s only needed in SSR, right? For me, the easiest way to do so is having a different entry point. That’s my point.

[cristianbote]

Got it @David. Yeah, I think that would be great to avoid the bloat.

[luisherranz]

What do you think about this guys?

AMP package

By the way, I was thinking that maybe doing the AMP-aware logic based on state.amp is not safe enough. If a user uses by mistake some AMP settings in his main site it will break the site. For example: const settings = { name: "main", state: { frontity: { url: "https://domain.com", }, amp: { ssrFallback: true // <- Breaks the site because `state.amp` is truthy } }, packages: [ // ... ], } What about state.amp?.isAmp: const Image = ({ state, src }) => …

[David]

OK, I spent some time thinking about this and, from my point of view, the best option would be to use state.frontity.mode.

First, because it would be a setting that would change the behavior of all the packages installed; therefore, it makes sense to include it in the frontity namespace.

Second, because if we add different “modes” in the future, for me it’s not a good idea to have a different property or flag for each of them, as it should not be possible to have two different modes working at the same time, right? The state.frontity.mode prop would ensure that only a single mode is active.

In the case we want to implement props like state.amp.ssrFallback, what that property could do is to change the “mode” Frontity works in case some condition is reached.

And this is my opinion.

[luisherranz]

Uhm… that is a good way to think about it. Thanks @david!

I agree that if we end up with different modes, it would be nicer to have a common property.

For example, a package that wants to do something only for the default mode would use:

if (state.frontity.mode === "default") {
  // ...
}

And if not it would have to know about all modes:

if (!state.frontity.isAmp && !state.frontity.isAnother && !state.frontity.isYetAnother) {
  // ...
}

That doesn’t scale.

The only limitation would be if there is a need to have two of those “modes” at the same time.

state.frontity.isAmp = true;
state.frontity.isAnother = true;

But I guess that makes less sense because mode will refer to the “rendering mode”, which will usually be only one.

@David

In the case we want to implement props like state.amp.ssrFallback, what that property could do is to change the “mode” Frontity works in case some condition is reached.

Could you please elaborate this one? I didn’t get it

[David]

@luisherranz

Could you please elaborate this one? I didn’t get it

Haha, sorry. I think I didn’t choose the right words.

I just wanted to say that it would be useful that packages could change the Frontity mode on their own. And that behavior could be controlled using package-specific settings.

Maybe state.apm.ssrFallback is not a good example, I don’t know how that feature is going to be implemented.

[SantosGuillamot]

I like using frontity.mode, you convinced me David .

Just one thing, if we use it, should we make it more explicit and not use just mode? I am wondering that we are also talking about modes with the Embedded and Decoupled approaches for example, and I guess that’s different right?

[luisherranz]

@SantosGuillamot

I am wondering that we are also talking about modes with the Embedded and Decoupled approaches for example, and I guess that’s different right?

Yeah, that’s totally right. We have to come up with a name that is not already used and mode is already used for Embedded and Decoupled modes. It would be confusing.

We cannot use state.frontity.rendering either because it is already used with ssr and csr values.

Any other ideas?