code review 68920046: go-tour: serve all javascript in a concatenated file

go-tour: serve all javascript in a concatenated file

Load time went down to 539ms from 1.90s!

[francesc, 2014-02-27 00:01:08]

Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go-tour

[adg, 2014-02-27 00:05:33]

You don't close the files when reading them. Use ioutil.ReadFile instead,
in the loop.

On Thursday, 27 February 2014, <campoy@golang.org> wrote:

> Reviewers: adg,
>
> Message:
> Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go-tour
>
>
> Description:
> go-tour: serve all javascript in a concatenated file
>
> Load time went down to 539ms from 1.90s!
>
> Please review this at https://codereview.appspot.com/68920046/
>
> Affected files (+41, -23 lines):
>   M app.yaml
>   M gotour/tour.go
>   M template/index.tmpl
>
>
> Index: app.yaml
> ===================================================================
> --- a/app.yaml
> +++ b/app.yaml
> @@ -15,6 +15,7 @@
>    static_dir: talks
>  - url: /static/
>    static_dir: static
> +  application_readable: true
>  - url: /(.*|list|lesson/.*|compile|fmt|script\.js)
>    script: _go_app
>
> Index: gotour/tour.go
> ===================================================================
> --- a/gotour/tour.go
> +++ b/gotour/tour.go
> @@ -63,7 +63,7 @@
>         }
>         uiContent = buf.Bytes()
>
> -       return nil
> +       return initScript(root)
>  }
>
>  // initLessonss finds all the lessons in the passed directory, renders
> them,
> @@ -227,16 +227,51 @@
>         return true
>  }
>
> -func init() {
> +// initScript concatenates all the javascript files needed to render
> +// the tour UI and serves the result on /script.js.
> +func initScript(root string) error {
>         modTime := time.Now()
> +       b := new(bytes.Buffer)
> +
>         content, ok := static.Files["playground.js"]
>         if !ok {
> -               panic("playground.js not found in static files")
> +               return fmt.Errorf("playground.js not found in static
> files")
>         }
> +       b.WriteString(content)
> +
> +       // Keep this list in dependency order
> +       files := []string{
> +               "static/lib/jquery/jquery.min.js",
> +               "static/lib/jquery-ui/js/jquery-ui-1.10.3.min.js",
> +               "static/lib/angular/angular.min.js",
> +               "static/lib/codemirror/lib/codemirror.js",
> +               "static/lib/codemirror/addon/edit/matchbrackets.js",
> +               "static/lib/codemirror/mode/go/go.js",
> +               "static/lib/angular-ui/angular-ui.js",
> +               "static/js/app.js",
> +               "static/js/controllers.js",
> +               "static/js/directives.js",
> +               "static/js/services.js",
> +               "static/js/values.js",
> +       }
> +
> +       for _, file := range files {
> +               f, err := os.Open(filepath.Join(root, file))
> +               if err != nil {
> +                       return fmt.Errorf("couldn't open %v", file, err)
> +               }
> +               _, err = io.Copy(b, f)
> +               if err != nil {
> +                       return fmt.Errorf("error copying %v", file, err)
> +               }
> +       }
> +
>         http.HandleFunc("/script.js", func(w http.ResponseWriter, r
> *http.Request) {
>                 w.Header().Set("Content-type", "application/javascript")
>                 // Set expiration time in one week.
>                 w.Header().Set("Cache-control", "max-age=604800")
> -               http.ServeContent(w, r, "", modTime,
> strings.NewReader(content))
> +               http.ServeContent(w, r, "", modTime,
> bytes.NewReader(b.Bytes()))
>         })
> +
> +       return nil
>  }
> Index: template/index.tmpl
> ===================================================================
> --- a/template/index.tmpl
> +++ b/template/index.tmpl
> @@ -11,12 +11,9 @@
>      <link rel="stylesheet" href="/static/css/app.css" />
>      <link rel="stylesheet" href="/static/lib/codemirror/
> lib/codemirror.css">
>      <link href='http://fonts.googleapis.com/css?family=Inconsolata'
> rel='stylesheet' type='text/css'>
> -    <script src="/static/lib/jquery/jquery.min.js"></script>
> -    <script src="/static/lib/jquery-ui/js/jquery-ui-1.10.3.min.js"></
> script>
>  </head>
>
>  <body>
> -
>      <div class="bar top-bar">
>          <a class="left logo" href="/list">A Tour of Go</a>
>          <div table-of-contents-button=".toc"></div>
> @@ -26,25 +23,11 @@
>
>      <div ng-view ng-cloak class="ng-cloak"></div>
>
> -    <script src="/static/lib/angular/angular.min.js"></script>
> -
> -    <script src="/static/lib/codemirror/lib/codemirror.js"></script>
> -    <script src="/static/lib/codemirror/addon/edit/matchbrackets.js"><
> /script>
> -    <script src="/static/lib/codemirror/mode/go/go.js"></script>
> -    <script src="/static/lib/angular-ui/angular-ui.js"></script>
> -
> -    <script src="/static/js/app.js"></script>
> -    <script src="/static/js/services.js"></script>
> -    <script src="/static/js/controllers.js"></script>
> -    <script src="/static/js/directives.js"></script>
> -    <script src="/static/js/values.js"></script>
> -
> -    <script src="/script.js"></script>
> +    <script async src="/script.js"></script>
>      <script>
>      window.transport = {{.Transport}}();
>      window.socketAddr = "{{.SocketAddr}}";
>
> -
>      function highlight(selector) {
>          var speed = 50;
>          var obj = $(selector).stop(true, true)
> @@ -66,7 +49,6 @@
>          $(selector)[0].click();
>      }
>      </script>
> -
>  </body>
>
>  </html>
>
>
>

[francesc, 2014-02-27 00:16:37]

oops, done


On Wed, Feb 26, 2014 at 4:05 PM, Andrew Gerrand <adg@golang.org> wrote:

> You don't close the files when reading them. Use ioutil.ReadFile instead,
> in the loop.
>
>
> On Thursday, 27 February 2014, <campoy@golang.org> wrote:
>
>> Reviewers: adg,
>>
>> Message:
>> Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go-tour
>>
>>
>> Description:
>> go-tour: serve all javascript in a concatenated file
>>
>> Load time went down to 539ms from 1.90s!
>>
>> Please review this at https://codereview.appspot.com/68920046/
>>
>> Affected files (+41, -23 lines):
>>   M app.yaml
>>   M gotour/tour.go
>>   M template/index.tmpl
>>
>>
>> Index: app.yaml
>> ===================================================================
>> --- a/app.yaml
>> +++ b/app.yaml
>> @@ -15,6 +15,7 @@
>>    static_dir: talks
>>  - url: /static/
>>    static_dir: static
>> +  application_readable: true
>>  - url: /(.*|list|lesson/.*|compile|fmt|script\.js)
>>    script: _go_app
>>
>> Index: gotour/tour.go
>> ===================================================================
>> --- a/gotour/tour.go
>> +++ b/gotour/tour.go
>> @@ -63,7 +63,7 @@
>>         }
>>         uiContent = buf.Bytes()
>>
>> -       return nil
>> +       return initScript(root)
>>  }
>>
>>  // initLessonss finds all the lessons in the passed directory, renders
>> them,
>> @@ -227,16 +227,51 @@
>>         return true
>>  }
>>
>> -func init() {
>> +// initScript concatenates all the javascript files needed to render
>> +// the tour UI and serves the result on /script.js.
>> +func initScript(root string) error {
>>         modTime := time.Now()
>> +       b := new(bytes.Buffer)
>> +
>>         content, ok := static.Files["playground.js"]
>>         if !ok {
>> -               panic("playground.js not found in static files")
>> +               return fmt.Errorf("playground.js not found in static
>> files")
>>         }
>> +       b.WriteString(content)
>> +
>> +       // Keep this list in dependency order
>> +       files := []string{
>> +               "static/lib/jquery/jquery.min.js",
>> +               "static/lib/jquery-ui/js/jquery-ui-1.10.3.min.js",
>> +               "static/lib/angular/angular.min.js",
>> +               "static/lib/codemirror/lib/codemirror.js",
>> +               "static/lib/codemirror/addon/edit/matchbrackets.js",
>> +               "static/lib/codemirror/mode/go/go.js",
>> +               "static/lib/angular-ui/angular-ui.js",
>> +               "static/js/app.js",
>> +               "static/js/controllers.js",
>> +               "static/js/directives.js",
>> +               "static/js/services.js",
>> +               "static/js/values.js",
>> +       }
>> +
>> +       for _, file := range files {
>> +               f, err := os.Open(filepath.Join(root, file))
>> +               if err != nil {
>> +                       return fmt.Errorf("couldn't open %v", file, err)
>> +               }
>> +               _, err = io.Copy(b, f)
>> +               if err != nil {
>> +                       return fmt.Errorf("error copying %v", file, err)
>> +               }
>> +       }
>> +
>>         http.HandleFunc("/script.js", func(w http.ResponseWriter, r
>> *http.Request) {
>>                 w.Header().Set("Content-type", "application/javascript")
>>                 // Set expiration time in one week.
>>                 w.Header().Set("Cache-control", "max-age=604800")
>> -               http.ServeContent(w, r, "", modTime,
>> strings.NewReader(content))
>> +               http.ServeContent(w, r, "", modTime,
>> bytes.NewReader(b.Bytes()))
>>         })
>> +
>> +       return nil
>>  }
>> Index: template/index.tmpl
>> ===================================================================
>> --- a/template/index.tmpl
>> +++ b/template/index.tmpl
>> @@ -11,12 +11,9 @@
>>      <link rel="stylesheet" href="/static/css/app.css" />
>>      <link rel="stylesheet" href="/static/lib/codemirror/
>> lib/codemirror.css">
>>      <link href='http://fonts.googleapis.com/css?family=Inconsolata'
>> rel='stylesheet' type='text/css'>
>> -    <script src="/static/lib/jquery/jquery.min.js"></script>
>> -    <script src="/static/lib/jquery-ui/js/jquery-ui-1.10.3.min.js"></
>> script>
>>  </head>
>>
>>  <body>
>> -
>>      <div class="bar top-bar">
>>          <a class="left logo" href="/list">A Tour of Go</a>
>>          <div table-of-contents-button=".toc"></div>
>> @@ -26,25 +23,11 @@
>>
>>      <div ng-view ng-cloak class="ng-cloak"></div>
>>
>> -    <script src="/static/lib/angular/angular.min.js"></script>
>> -
>> -    <script src="/static/lib/codemirror/lib/codemirror.js"></script>
>> -    <script src="/static/lib/codemirror/addon/edit/matchbrackets.js"><
>> /script>
>> -    <script src="/static/lib/codemirror/mode/go/go.js"></script>
>> -    <script src="/static/lib/angular-ui/angular-ui.js"></script>
>> -
>> -    <script src="/static/js/app.js"></script>
>> -    <script src="/static/js/services.js"></script>
>> -    <script src="/static/js/controllers.js"></script>
>> -    <script src="/static/js/directives.js"></script>
>> -    <script src="/static/js/values.js"></script>
>> -
>> -    <script src="/script.js"></script>
>> +    <script async src="/script.js"></script>
>>      <script>
>>      window.transport = {{.Transport}}();
>>      window.socketAddr = "{{.SocketAddr}}";
>>
>> -
>>      function highlight(selector) {
>>          var speed = 50;
>>          var obj = $(selector).stop(true, true)
>> @@ -66,7 +49,6 @@
>>          $(selector)[0].click();
>>      }
>>      </script>
>> -
>>  </body>
>>
>>  </html>
>>
>>
>>


-- 
--
Francesc Campoy
http://campoy.cat

[adg, 2014-02-27 01:00:04]

LGTM

[francesc, 2014-02-27 01:18:48]

*** Submitted as https://code.google.com/p/go-tour/source/detail?r=b65851ce9f0a
***

go-tour: serve all javascript in a concatenated file

Load time went down to 539ms from 1.90s!

LGTM=adg
R=adg
CC=golang-codereviews
https://codereview.appspot.com/68920046