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

Design Review 2017-04-05 (server side rendering, origin whitelist for experiments) #8193

Closed
mrjoro opened this issue Mar 16, 2017 · 8 comments
Assignees
Milestone

Comments

@mrjoro
Copy link
Member

mrjoro commented Mar 16, 2017

The AMP Project holds weekly engineering design reviews on Wednesdays @ 1pm Pacific via video conference using Google Hangouts.

Participation by the entire AMP Project community is encouraged.

This issue will be updated by 1pm Pacific on the Monday before the design review with links to the design docs that will be discussed. Please read through the design docs that will be discussed before attending the design review.

If you have a design you'd like to bring to design review, please read through the guidelines. When you are ready to bring your design to design review (after following the guidelines) update the appropriate Design Review GitHub issue with a link to your design doc and a brief description by 1pm Pacific on Monday of the week you'd like to present your design.

@mrjoro mrjoro added this to the Docs Updates milestone Mar 16, 2017
@mrjoro mrjoro self-assigned this Mar 16, 2017
@mrjoro
Copy link
Member Author

mrjoro commented Mar 16, 2017

@honeybadgerdontcare and @dvoytenko are tentatively scheduled to talk about server-side rendering

@dreamofabear
Copy link

Would like to talk about origin whitelists for AMP experiments: #8331

@honeybadgerdontcare
Copy link
Contributor

honeybadgerdontcare commented Apr 3, 2017

Can talk about server-side rendering in AMP caches: #8566

@jasti
Copy link
Contributor

jasti commented Apr 5, 2017

@clawr, can you, by any chance, come to tomorrow's design review to talk about #8515?
Thanks.

@jasti
Copy link
Contributor

jasti commented Apr 5, 2017

@clawr sounds like tomorrow's agenda is packed. Would you please be able to present #8515 at next week's design review - #8582? Thanks!

@mrjoro
Copy link
Member Author

mrjoro commented Apr 5, 2017

It sounds like the issues @derekcecillewis and @jasti mentioned will be covered in the future.

This week we'll start with @honeybadgerdontcare's server-side rendering and if there's time we'll cover @choumx's origin whitelist for AMP experiments.

@mrjoro
Copy link
Member Author

mrjoro commented Apr 5, 2017

Notes on the server-side rendering:

  • Is this intended for any server-side rendering of AMP pages?
    • No, just for AMP caches after they’ve validated the doc; it’s basically inlining part of the AMP runtime.
    • The docs won’t be valid AMP after this.
  • Are we going to document the rules in code (vs. just in an English design doc)?
    • Dima has thought about pulling out the applyLayout JS, and caches can implement that on the server
  • Need to be aware of when the AMP JS changes (applyLayout) to update the servers to do it as well
    • This might not be that hard to keep track of because we normally validate the server-side validators when we change applyLayout as well.
    • Malte: it’d be pretty hard to change applyLayout at this point anyway
    • What happens if we e.g. add a new layout?
    • There is some value in partial implementation of server-side rendering even if we don't do everything; even if we don’t remove all boilerplate, we could replace parts of the page that are expensive
  • When can we have an experiment to determine how much this saves for speed?
    • We currently expect it’d be roughly twice as fast
    • AI(@honeybadgerdontcare): get results and bring them back to a design review
  • Would be good to have golden files to compare the DOM the client AMP JS creates. Vs. what the server optimization creates.
  • Would this affect local development?
    • Caches already do some transformations, so it doesn’t really change today
    • input format to a compiler (like Closure, input and output are the same, but one is more optimized; exploit well-known behavior to create a more efficient representation of the input)
    • Explicit goal is that it produces the exact same rendering in the browser
  • Is it worth it to strip out the applyLayout code from JS (have a different version of JS that doesn’t have applyLayout)?
    • Not too many bytes saved (may not be worth it)
  • Are we building an API for this?
    • Probably not worth it

@mrjoro
Copy link
Member Author

mrjoro commented Apr 5, 2017

Notes on origin whitelist for experiments:

  • Benefit of this over per-page experiments if they have to include meta on each page anyway?
    • There’s an expiration for it
    • We have their contact info to be able to solicit feedback
    • Doc-level is “it’s about to launch, certainty is 99%”; only for a controlled set of experiments, the last step just before launch
    • Dima: we already have doc level experiments, it’s just not signed & are not restricted to origins
  • Why would we only restrict certain publishers to something?
    • We do this when we really think something is going to change, but really want production feedback
  • This is different than amp-experiment; in this case the experiment is on for everyone who uses the page
    • John: say there’s a change in amp-sticky-ad, and he wants to randomize giving the new thing to half of his visitors and the old thing to the other half of his visitors
    • Malte: this is a different type of experiment than what we are changing (though that could be a good feature)
  • You can’t currently do a %-age on the meta tag, but Dima thinks it’d be easy to do
  • Terminology is also something we should work on (amp-experiment vs. these experiments)
  • Dima: something like this is needed, but how do we support it when web crypto is not supported
    • Every modern browser supports it
    • API is asynchronous vs. our current system is synchronous (and we start hating everything that’s async)
  • Publishers would have to be careful not to break if the experiment isn’t enabled (not just for this case); this hints at the need to be able to support the experiment that John mentioned (though this is advanced stuff)
  • Will’s main use case right now is for rolling out amp-bind
    • Malte: we don’t want to put this on the critical path of anything we want to launch soon
  • Dima: we could enable experiments like this via the cache instead (e.g. using hidden tags)
    • Would have to share the definition with other caches, which could be difficult
    • Malte isn’t sold on this idea
  • We’d also need to build the registration UI + nag emails when they’re going to expire/etc.

@mrjoro mrjoro closed this as completed Apr 10, 2017
@mrjoro mrjoro changed the title Design Review 2017-04-05 Design Review 2017-04-05 (server side rendering, origin whitelist for experiments) Jun 11, 2017
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

5 participants