Amphtml: AMP validator produces a warning for script containing mustache bindings nested in template

Created on 9 Dec 2019 · 8Comments · Source: ampproject/amphtml

What's the issue?

We noticed our pages have a warning regarding invalid JSON saying:

⚠️ The script tag contains invalid JSON that cannot be parsed

I'd like to stress that the invalid JSON is nested amp-lists template and won't be used on page as-is. At the point when template is evaluated the JSON should be valid.
Consider the following example to better understand the issue (some tags and attributes were omitted to simplify the markup):

<amp-list src="...">
  <template type="amp-mustache">
    <amp-analytics>
    <script type="application/json">
     {{{ json }}}
    </script>
    <amp-analytics/>
  </template>
</amp-list>

As you can see the script content will be fetched from server and placed into the script tag.

Although it's just a warning Google Search Console is saying the following regarding affected pages:

And we want our pages to be fully AMP compliant and valid.

How do we reproduce the issue?

Go to https://validator.ampproject.org
Place into text field the following snippet:

<!doctype html>
<html ⚡>
<head>
  <meta charset="utf-8">
  <link rel="canonical" href="self.html" />
  <meta name="viewport" content="width=device-width,minimum-scale=1">
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  <script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
  <script async custom-element="amp-list" src="https://cdn.ampproject.org/v0/amp-list-0.1.js"></script>
<script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js"></script>
</head>
<body>
<amp-list
  width="auto"
  height="100"
  layout="fixed-height"
  src="/example.json"
>
  <template type="amp-mustache">
    <amp-analytics>
    <script type="application/json">
     {{{ json_data }}}
    </script>
    <amp-analytics/>
  </template>
</amp-list>
</body>
</html>

3 Verify the warning appears.

What browsers are affected?

Any

Which AMP version is affected?

Any

When Possible Bug caching

Source

OEvgeny

All 8 comments

Friendly ping to caching working group (cc @honeybadgerdontcare)

It'd be nice to know you thoughts regarding this.

OEvgeny on 16 Dec 2019

@honeybadgerdontcare I see it's P3. Does Caching WG accept contributions for such issues?

OEvgeny on 17 Dec 2019

@OEvgeny Sorry, I thought I left a comment. It is a P3 and we do accept contributions.

It would be good to determine the exact syntax for what should be valid. Is it all on one line (e.g. <script...>{{{ json }}}</script>? Is it always json as the variable name? In other words, we need to know what exactly the exception would be for the script data to not be parsable JSON.

honeybadgerdontcare on 17 Dec 2019

Here's some documentation on contributing to the AMP project, and specifically contributing code.

honeybadgerdontcare on 17 Dec 2019

👍1

Thanks, the information you provided is very helpful.

Personally, I'd exclude JSON scripts in templates from validation. They may contain bindings and might be invalid because of the fact. Probably we could try to replace all bindings with something else (e.g. {} empty object). This won't cover all the cases, but at least it covers simple ones.
The variable name can be different for different use-cases, I believe.

Consider the following examples of using mustache bindings to get valid json:

{{{ jsonStart }}}{{{ jsonEnd }}}

1:
  - jsonStart = '{"foo": "bar",'
  - jsonEnd = '"bar", 1}'

2.
  - jsonStart = '{'
  - jsonEnd = '}'

3.
  - jsonStart = '1'
  - jsonEnd = ''

{ "some": { "nested": { "field": {{{ jsonNestedData }}} } } }

1:
  - jsonNestedData = '{ "foo": "bar" }'

2:
  - jsonNestedData = '1'

3:
  - jsonNestedData = '""'

{{{ jsonData }}}

1. 
  - jsonNestedData = '{}'

2. 
  - jsonNestedData = '""'

3. 
  - jsonNestedData = '1'

These are cases I have in mind, but there might be more. That's why I think the check should not be applied to templates at all. There might be a lot of valid cases for JSON templating using mustache.

OEvgeny on 18 Dec 2019

@OEvgeny After discussing this offline we're thinking of not emitting the warning for JSON parsing when it's a descendant of template. Would that work for you?

Since this is just a warning we're continuing with our view of it as a P3: when possible.

honeybadgerdontcare on 23 Dec 2019

👍1

@honeybadgerdontcare thanks for update. Yes, that'd work for me.

OEvgeny on 24 Dec 2019

This has been fixed.