Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for validator rule for RTC config in head #9800

Closed
bradfrizzell opened this issue Jun 8, 2017 · 8 comments
Closed

Add support for validator rule for RTC config in head #9800

bradfrizzell opened this issue Jun 8, 2017 · 8 comments
Assignees

Comments

@bradfrizzell
Copy link
Contributor

As part of adding support for RTC, publishers will be asked to include a config in their head, formatted like the following example. We need validator support added for this.

<script type="application/ld+json" id="amp-rtc">
  {
    "doubleclick": {
      "url": "https://pub.com/rtc-endpoint",
      "sendAdRequestOnFailure": true
    },
    "fake-network": {
      "url": "https://pub.com/rtc-endpoint-fake-network",
      "sendAdRequestOnFailure": false
    }
  }
</script>
  • Keys of the object must be the name of an ad network that supports AMP RTC.
  • The value of ‘url’ must be a valid url.
  • ‘sendAdRequestOnFailure’ is an optional member of the inner objects.
  • The id of the script must be amp-rtc.
  • The type of the script must be application/ld+json
  • Script must be in head.
@Gregable
Copy link
Member

Gregable commented Jun 8, 2017

If it's OK with you, we'll leave validation of the json contents up to the javascript library, rather than making it a requirement for the document to be valid AMP, thus we'll validate:

  • script element, in the head
  • type="application/json"
  • id="amp-rtc"

@jridgewell
Copy link
Contributor

Why does it need an id?

@Gregable
Copy link
Member

Gregable commented Jun 8, 2017

@bradfrizzell , I don't know the RTC reason.

For validation, it helps distinguish this from other script tags, for producing good errors. For example, if the user left off type=application/json by accident but included the id, we can produce an error that directs the user to correcting the type. Without the id, we'd just need to give a generic "javascript is not allowed" type error.

@bradfrizzell
Copy link
Contributor Author

The ID, from our end, was so we can grab it with getElementById. Is there something else I should do instead? Is there an issue with requiring an ID?

@Gregable
Copy link
Member

Gregable commented Jun 9, 2017

This is the exact same thing we do for amp-access, I think id is fine.

@Gregable
Copy link
Member

Gregable commented Jun 9, 2017

The changes are committed in #9831, The type attribute will be application/json, not application/ld+json which is a separate thing. We will not validate the actual json contents - anything is allowed. That'll be a runtime issue. Let me know if there is anything more we need here.

Note that this won't be released for another 2 weeks or so.

@Gregable Gregable closed this as completed Jun 9, 2017
@bradfrizzell
Copy link
Contributor Author

Thanks!

@Gregable
Copy link
Member

This change has been released to the validator and will be live within an hour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants