Skip to content
This repository

Use script element with async if we can detect support #854

Closed
wants to merge 7 commits into from

4 participants

Andy Hume Matt Chadburn Grant Klopper Kaelig
Andy Hume
ahume commented

Adds BrowserSupport module which looks at request headers set by Varnish. These can then be used in templates to adapt HTML better for certain browsers.

This change won't do anything until we roll out new Varnish config.

Matt Chadburn

looks ok, but we want to vary the response on something (Ie. x-gu-feature) don't we?

Andy Hume
ahume commented

I think we just vary the cache key in Varnish on this header as well.

Andy Hume
ahume commented

Ohh, you mean for other proxies?

Kaelig kaelig commented on the diff
.../app/views/fragments/commonJavaScriptSetup.scala.html
((20 lines not shown))
105 106
106   - document.getElementsByTagName("head")[0].appendChild(script);
107   -
108   - })(guardian.isModernBrowser);
  107 + var script = document.createElement('script');
  108 + script.async = 'async';
1
Kaelig Collaborator
kaelig added a note

TL;DR: we can probably get rid of this line

Parser-inserted script elements will be executed asynchronously whether the async attribute is set or not. Unless we want full support for Firefox 3.6 and OmniWeb 622 (which need this line).

Spec says:
"If was-parser-inserted is true and the element does not have an async attribute, then set the element's "force-async" flag to true."
http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html

More on that in Steve Souders' research: http://www.stevesouders.com/blog/2012/01/13/javascript-performance/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Grant Klopper
Owner

Going to kill this off for now. We are still some ways off doing this, and I think the place to start would be in the Varnish config (setting device type).

Grant Klopper gklopper closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
2  common/app/assets/javascripts/modules/analytics/omniture.js
@@ -133,6 +133,8 @@ define([
133 133
134 134 s.prop68 = detect.getConnectionSpeed(w.performance, null, true);
135 135
  136 + s.prop69 = (document.querySelector('[data-app-js]') !== null) ? 'script async' : 'script appendChild';
  137 +
136 138 if (config.page.webPublicationDate) {
137 139 s.prop30 = 'content';
138 140 } else {
15 common/app/common/BrowserSupport.scala
... ... @@ -0,0 +1,15 @@
  1 +package common
  2 +
  3 +import play.api.mvc.RequestHeader
  4 +
  5 +
  6 +case class BrowserSupport(request: RequestHeader) {
  7 +
  8 + private def headerContains(header: String): Boolean = {
  9 + request.headers.get("X-Gu-Feature").map(_.split(",").contains(header)).getOrElse(false)
  10 + }
  11 +
  12 + lazy val AsyncScript = headerContains("AsyncScript")
  13 + lazy val HTMLPreparser = headerContains("HTMLPreparser")
  14 +
  15 +}
29 common/app/views/fragments/commonJavaScriptSetup.scala.html
@@ -14,7 +14,8 @@
14 14 gallery: '@Static("stylesheets/gallery.min.css")',
15 15 story: '@Static("stylesheets/story.min.css")',
16 16 video: '@Static("stylesheets/video.min.css")'
17   - }
  17 + },
  18 + config: @fragments.javaScriptConfig(item, switches)
18 19 },
19 20 curl = {
20 21 baseUrl: '@{Configuration.assets.path}javascripts',
@@ -93,18 +94,22 @@
93 94 }
94 95 })();
95 96
96   - (function(isModern) {
97   -
98   - if (!isModern) { return false; }
  97 +</script>
99 98
100   - guardian.config = @fragments.javaScriptConfig(item, switches);
  99 +@if(BrowserSupport(request).AsyncScript) {
  100 + <script src="@Static("javascripts/bootstraps/app.js")" async data-app-js></script>
  101 +} else {
  102 + <script>
  103 + (function(isModern) {
101 104
102   - var script = document.createElement('script');
103   - script.async = 'async';
104   - script.src = '@Static("javascripts/bootstraps/app.js")';
  105 + if (!isModern) { return false; }
105 106
106   - document.getElementsByTagName("head")[0].appendChild(script);
107   -
108   - })(guardian.isModernBrowser);
  107 + var script = document.createElement('script');
  108 + script.async = 'async';
  109 + script.src = '@Static("javascripts/bootstraps/app.js")';
109 110
110   -</script>
  111 + document.getElementsByTagName("head")[0].appendChild(script);
  112 +
  113 + })(guardian.isModernBrowser);
  114 + </script>
  115 +}

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.