amp-wp icon indicating copy to clipboard operation
amp-wp copied to clipboard

Include AMP content in REST API responses for embedding (AMP in PWA)

Open westonruter opened this issue 6 years ago • 36 comments

In #1013 a script for the Shadow AMP API is registered. To facilitate AMP in PWA there needs to be an easy way to obtain AMP content to embed into a page. One easy way would just be to add an amp property to go alongside the existing raw and rendered properties of the content field.

The logic used to obtain the content.amp value would be similar to:

https://github.com/ampproject/amp-wp/blob/e2c5b193a6350282cf9387c7890fc99c97ab0156/includes/utils/class-amp-validation-utils.php#L357-L367

The REST API endpoint is extensible for plugins to add a new property here. For example, the rest_prepare_{$this->post_type} filter could be used. Nevertheless, the schema should also be updated to advertise this new property.

westonruter avatar Mar 12 '18 18:03 westonruter

See also https://github.com/WordPress/gutenberg/issues/7668

westonruter avatar Jul 10 '18 01:07 westonruter

A second endpoint for the tree shaken CSS would be great too

ataylorme avatar Jun 25 '19 07:06 ataylorme

An array of scripts would be useful as well:

{
  ...
  content: {
    rendered: "....<iframe src="https://youtube....></iframe>",
    amp: "...<amp-youtube ...>..."
  },
  amp: {
    styles: "....",
    scripts: [
      {
        "custom-element": "amp-youtube",
        src: "https://cdn.ampproject.org/v0/amp-youtube-0.1.js"
      }
    ]
  }
}

luisherranz avatar Jun 25 '19 09:06 luisherranz

I'd love to share another point here, just in case it matters during the implementation: This content.amp field is not only useful for the Shadow AMP API, but also for Headless WP frameworks used to render full AMP pages, like Next.js 8+ or Frontity's upcoming support.

luisherranz avatar Jul 17 '19 09:07 luisherranz

I've started a PR (https://github.com/ampproject/amp-wp/pull/2827) to work on this.

luisherranz avatar Jul 17 '19 15:07 luisherranz

Nevertheless, the schema should also be updated to advertise this new property.

I don't currently see a way to do that when adding the AMP content under content.amp. It seems only top-level fields added via register_rest_field are allowed to extend the schema.

swissspidy avatar Jul 17 '19 20:07 swissspidy

Extracting https://github.com/ampproject/amp-wp/pull/2827#discussion_r304967583 from PR from @luisherranz for discussion here:


Ok, so now this is getting very interesting. We have many options here. I'll try to summarize the options and the decisions we need to make.

Options for AMP output:

  1. content.amp: full document AMP.
  2. Maintain content.amp but add new field amp:
    • content.amp: only the_content() converted to AMP
    • amp.styles: tree-shaken CSS
    • amp.scripts: array of used scripts
  3. Put everything in content.amp:
    • content.amp.body: only the_content() converted to AMP
    • content.amp.styles: tree-shaken CSS
    • content.amp.scripts: array of used scripts
  4. Optional using the query (see below).

Options for the opt-in query:

  1. add a separate endpoint and use _links to make it embeddable with _embed.
  2. add a new query, for example _amp.
    1. make it boolean (include it or not).
    2. add option for output format, for example:
    • _amp=full: full document.
    • _amp=body: only the_content.

The only thing I have strong feelings against is to use _embed for the opt-in. If this is expensive, I think it should be a separate param. Most headless themes are probably using _embed on each request, so I don't see why they would need amp unless they are actively using AMP.

How are you intending to incorporate this into Frontity?

I think both options suit different use cases:

  • Do you have a PWA and want to insert the content using AMP instead of HTML because it has some nice performance features? Use the Shadow AMP API.
  • Do you want to generate full AMP pages using a decoupled solution like Frontity or Next? Insert the content and add the scripts and styles to your head.

If we were forced to do only one, I'd rather go with the separate body content, scripts, and styles because as @ataylorme said it's easier to combine them to generate the full document than the other way around.


I guess one interesting question would be: can the Shadow AMP API be used inside an AMP document?

westonruter avatar Jul 18 '19 16:07 westonruter

And https://github.com/ampproject/amp-wp/pull/2827#discussion_r304972728 from @swissspidy:

Should we just use register_rest_field and then opt-in is done via using the _fields param?

@swissspidy Is this done elsewhere with the _fields param? Isn't it normally used only for reducing the number of fields, rather than adding to the number of fields included?

westonruter avatar Jul 18 '19 16:07 westonruter

A couple of different use cases for the different options, also extracted from the PR comments:

1. Full AMP document

Do you have a PWA and want to insert the content using AMP instead of HTML because it has some nice performance features? Use the full AMP document and the Shadow AMP API.

2. Body content + styles + scripts

Do you want to generate AMP pages using a decoupled solution like Frontity or Next? Insert the body content in the correct place and add the scripts and styles to your head.

luisherranz avatar Jul 18 '19 16:07 luisherranz

For what I've seen so far there's no easy way to include a full AMP document inside another full AMP document, so using the Full AMP document for the use case 2 doesn't seem possible.

luisherranz avatar Jul 18 '19 16:07 luisherranz

I guess one interesting question would be: can the Shadow AMP API be used inside an AMP document?

The only way to do so would be to use an amp-iframe at present, but this is not ideal. There should really be some dedicated amp-shadow-doc element which would allow for this.

westonruter avatar Jul 18 '19 16:07 westonruter

  1. Body content + styles + scripts

I think this is going to be the preferred format for AMP data being included in the post entity of a REST API response JSON. The reason is that AMP Shadow API prefers to be populated with streaming: https://github.com/ampproject/amphtml/blob/master/spec/amp-shadow-doc.md#fetching-and-attaching-shadow-docs

So streaming entails that the response is HTML not JSON.

In a non-shadow context, when the AMP markup is being assembled into the page server-side, then there is no streaming preference. Also, when being assembled into an existing page, this already requires that the fields be broken up into content, style, and scripts. So this makes even more sense to prefer separation in the post entity.

westonruter avatar Jul 18 '19 17:07 westonruter

  1. Full AMP document

Nevertheless, we can also support the Shadow use case by having a separate link in the response which has a URL pointing to the standalone content rendered in a valid AMP document.

westonruter avatar Jul 18 '19 17:07 westonruter

For content + styles + scripts, take a look at this: https://github.com/ampproject/amp-wp/pull/2827#issuecomment-513090982

westonruter avatar Jul 19 '19 05:07 westonruter

Looks great to me so far.

I've also added the version numbers to the scripts array following the spec and removed the check of content.rendered now that we are using $post->post_content.

luisherranz avatar Jul 19 '19 07:07 luisherranz

Is this done elsewhere with the _fields param? Isn't it normally used only for reducing the number of fields, rather than adding to the number of fields included?

Yeah that's true. Not really an option then. It would have been an easy way for adding the proper schema though.

swissspidy avatar Jul 24 '19 10:07 swissspidy

Maybe it's ultimately overkill to try to omit the content.amp by default, as long as the AMP processing is only being done when content is among the _fields being included.

westonruter avatar Jul 24 '19 15:07 westonruter

It looks like extending the content schema to include the amp property was not possible yet so I've opened a core ticket to add a filter in WP_REST_Posts_Controller::get_item_schema() and submitted a patch myself: https://core.trac.wordpress.org/ticket/47779

Then I've added the filter to our AMP_REST_API class with the new, extended schema.

The new schema looks like this:

  "content": {
    "description": "The content for the object.",
    "type": "object",
    "context": ["view", "edit"],
    "properties": {
      "raw": {
        "description": "Content for the object, as it exists in the database.",
        "type": "string",
        "context": ["edit"]
      },
      "rendered": {
        "description": "HTML content for the object, transformed for display.",
        "type": "string",
        "context": ["view", "edit"],
        "readonly": true
      },
      "block_version": {
        "description": "Version of the content block format used by the object.",
        "type": "integer",
        "context": ["edit"],
        "readonly": true
      },
      "protected": {
        "description": "Whether the content is protected with a password.",
        "type": "boolean",
        "context": ["view", "edit", "embed"],
        "readonly": true
      },
      "amp": {
        "description": "The AMP content for the object.",
        "type": "object",
        "context": ["view", "edit"],
        "readonly": true,
        "properties": {
          "markup": {
            "description": "HTML content for the object, transformed to be valid AMP.",
            "type": "string",
            "context": ["view", "edit"],
            "readonly": true
          },
          "styles": {
            "description": "An array of tree-shaken CSS styles extracted from the content.",
            "type": "array",
            "items": {
              "type": "string"
            },
            "context": ["view", "edit"],
            "readonly": true
          },
          "scripts": {
            "description": "An object of scripts, extracted from the AMP elements and templates present in the content.",
            "type": "object",
            "context": ["view", "edit"],
            "readonly": true,
            "additionalProperties": {
              "type": "object",
              "context": ["view", "edit"],
              "readonly": true,
              "properties": {
                "src": {
                  "type": "string",
                  "description": "The source of the script.",
                  "context": ["view", "edit"],
                  "readonly": true
                },
                "runtime_version": {
                  "type": "string",
                  "description": "The runtime version of AMP used by the script.",
                  "context": ["view", "edit"],
                  "readonly": true
                },
                "extension_version": {
                  "type": "string",
                  "description": "The version of the script itself.",
                  "context": ["view", "edit"],
                  "readonly": true
                },
                "async": {
                  "type": "boolean",
                  "description": "Whether or not the script should be loaded asynchronously.",
                  "context": ["view", "edit"],
                  "readonly": true
                },
                "extension_type": {
                  "type": "string",
                  "enum": ["custom-template", "custom-element"],
                  "description": "Type of the script, either a template or an element.",
                  "context": ["view", "edit"],
                  "readonly": true
                }
              }
            }
          }
        }
      }
    }
  }

luisherranz avatar Jul 26 '19 12:07 luisherranz

Disable AMP content in REST API if website experience is not enabled

@westonruter I'd love to know more about this.

The website experience needs to be enabled to be able to use the sanitizers or is this something you think will lead to a better user experience?

luisherranz avatar Jul 26 '19 15:07 luisherranz

@luisherranz Yes. When \AMP_Options_Manager::is_website_experience_enabled() is false, that means the site is only using AMP Stories but not AMP for the rest of the website.

Such a check can be added to rest_api_init(), i.e. short-circuit when the website experience is disabled, unless the post type equals \AMP_Story_Post_Type::POST_TYPE_SLUG.

swissspidy avatar Jul 26 '19 15:07 swissspidy

What happens if someone doesn't want to use AMP in their website (PHP theme) but for some reason wants to use it in another place where the content is fetched via the REST API.

I mean, why adding an extra limitation when we already are making the content.amp opt-in only though a query param?

Not that I disagree, but I'm trying to understand :)

luisherranz avatar Jul 26 '19 16:07 luisherranz

What happens if someone doesn't want to use AMP in their website (PHP theme) but for some reason wants to use it in another place where the content is fetched via the REST API.

If they want to do that they could uncheck all of the templates:

image

I mean, why adding an extra limitation when we already are making the content.amp opt-in only though a query param?

What if we discontinued that? Per https://github.com/ampproject/amp-wp/issues/1014#issuecomment-514683394

My concern is discoverability of the AMP fields on the REST API responses. If you have to supply a secret query parameter, then this would seem to be a violation of how the REST API was designed (e.g. HAL).

westonruter avatar Jul 26 '19 23:07 westonruter

Ok, I understand. The _amp query is probably not a good idea then.

Still, having to enable the website experience to get a field in the REST API doesn't look like the most intuitive approach to me.

What about adding a setting for the REST API? Only when that setting is checked we insert the field and extend the schema.

We can even encourage people to use a cache plugin for the REST API, like you do with object cache. Either in the description or in a warning notice.

Something like this:

Screen Shot 2019-07-27 at 17 10 20

luisherranz avatar Jul 27 '19 15:07 luisherranz

Usually the website experience is checked, however. If you uncheck the website experience then that entails you don't want your (non-story) content to be available in AMP, so I think it makes sense for that to control whether or not the REST API fields are included.

Also, I think my concern about AMP content being expensive to generate is probably overblown. Perhaps just include but default but then allow omitting via some AMP-specific PHP filter.

westonruter avatar Jul 27 '19 20:07 westonruter

It's obviously your take guys, but I still think that if you want REST API support without affecting your site, having to enable website and disable all the templates is far less intuitive than having a REST API checkbox.

Anyway, let me know your decision and I'll continue the development :)


Apart from that, @westonruter how is your idea for this? Is it a new field?

Add link for where the rendered AMP content can be fetched as an entire valid AMP document (for Shadow AMP document purposes).

luisherranz avatar Jul 29 '19 10:07 luisherranz

It's obviously your take guys, but I still think that if you want REST API support without affecting your site, having to enable website and disable all the templates is far less intuitive than having a REST API checkbox.

It's not entirely intuitive, but I think it's intuitive enough, given that this should be a very rare scenario. Let's go ahead and gate the inclusion of content.amp based on \AMP_Options_Manager::is_website_experience_enabled() being true. As part of this, let's remove this note:

image

https://github.com/ampproject/amp-wp/blob/1b8afd930a6c3512846ecae0ef767e78275d7a57/includes/class-amp-theme-support.php#L831

westonruter avatar Jul 29 '19 17:07 westonruter

Add link for where the rendered AMP content can be fetched as an entire valid AMP document (for Shadow AMP document purposes).

This was the route I was going for a standalone plugin I was working on before, so I'll work on extracting that code to incorporate into your PR.

westonruter avatar Jul 29 '19 17:07 westonruter

@kadamwhite re: https://github.com/ampproject/amp-wp/pull/2827#issuecomment-517396233

Would love your thoughts on the best way for a client to opt-in to include AMP content in the REST API response. See notes above about a special _amp query param, the schema, and HAL.

westonruter avatar Aug 01 '19 21:08 westonruter

@westonruter Sorry, missed the ping. I would think the most conceptually accurate way to handle this would be to introduce a new context=amp. It's possible that we would still need to introduce the ability to filter the schema, but I think that Context is the right method to use to signal that we will be exposing and returning alternative data structures, as this is already the mechanism we use to signal the presence or lack of the raw content (for example).

kadamwhite avatar Aug 22 '19 00:08 kadamwhite

I would continue, that if context=amp, rather than returning a (mostly duplicative) content.rendered alongside the new content.amp, I would consider it more appropriate to return the AMP content as content.rendered.

Additional AMP properties like CSS would still need to be added on whenever they don't belong as part of the content proper, but unless there's a situation where both content.rendered and content.amp would be required at once, it feels like overkill to include both.

kadamwhite avatar Aug 22 '19 00:08 kadamwhite