Repository: MoOx/phenomic
Pull request #904 (2016-12-02 22:46:04)

Prevent duplicate loading of JS files
When doing a build react-helmet is loading js files once in the head and once in the body.  React helmet manages the head and thus this script toComponent() should be in the head.

MoOx: Hi. Is this really creating an issue? Can you give more details about this?
timothycoy: Yes, particularly because of the base head is not part of the template, so the best way to load js files in the head is to use react-helmet.  But then, because of this issue it's loading all the scripts twice.

Steps to reproduce:
npm run build
npm install --save-dev http-server
./node_modules/.bin/http-server dist

Screenshots:
<img width="894" alt="screen shot 2016-12-03 at 10 02 55 am" src="https://cloud.githubusercontent.com/assets/2123504/20860184/0b9d0624-b940-11e6-9c97-1f55cd9bf573.png">
<img width="780" alt="screen shot 2016-12-03 at 10 03 10 am" src="https://cloud.githubusercontent.com/assets/2123504/20860185/0f9f187a-b940-11e6-9e05-eabc5eaa6d67.png">
MoOx: Did you check if there are options in react-helmet about that?
I am not a fan on pushing by default script into the head as it's not a good practice (slow down the first paint)
timothycoy: react-helmet is specifically for managing the head, so there are no options for that.  Also, sometimes you want to load scripts in the head.  For example, a script that has logic, which sometimes redirects to another page.  For example: optimizely.  Another example would be new relic, which you want to get accurate page load timings.

Now, with that said there are a number of other react libraries such as react-async-script or ReactScriptLoader that can be used to asynchronously load external scripts in the body.  But, these can easily be added as part of the template by the developer as needed.

arlair: This change worked for me. My opinion is this commit sounds good as a temporary iterative step because currently the scripts are added to the head and the body. Then open a new enhancement issue to investigate body only script options.

I had a quick look and React-Helmet appears [hard coded to the headElement](https://github.com/nfl/react-helmet/blob/9a3fbdc063a6de3b68395767769f34e398fbe362/src/Helmet.js#L240) as far as I could tell. [headElement being set here](https://github.com/nfl/react-helmet/blob/9a3fbdc063a6de3b68395767769f34e398fbe362/src/Helmet.js#L240).
MoOx: I am ok with this changes if it brings more problem than it's supposed to solves. Can you fix the relevant tests please? If you need some help [reach us on gitter](https://gitter.im/MoOx/phenomic)
timothycoy: Done.
MoOx: Thanks! I will release this as a minor (not a patch) as some people might get unexpected results.

Patch:
diff --git a/src/components/Html/index.js b/src/components/Html/index.js
index 3ce20ec6..160ff14c 100644
--- a/src/components/Html/index.js
+++ b/src/components/Html/index.js
@@ -100,11 +100,11 @@ const Html = (props: Props) => {
             <link key={ "phenomic.css." + i } rel="stylesheet" href={ file } />
           ))
         }
+        { head.script.toComponent() }
       </head>
       <body>
         <div id="phenomic" dangerouslySetInnerHTML={{ __html: body }} />
         { props.renderScript() }
-        { head.script.toComponent() }
         {
           props.js.map((file, i) => (
             <script key={ "phenomic.js." + i } src={ file } />


Test patch:
diff --git a/src/components/Html/__tests__/__snapshots__/aphrodite.js.snap b/src/components/Html/__tests__/__snapshots__/aphrodite.js.snap
index 93c7fb61..003fc67c 100644
--- a/src/components/Html/__tests__/__snapshots__/aphrodite.js.snap
+++ b/src/components/Html/__tests__/__snapshots__/aphrodite.js.snap
@@ -23,6 +23,13 @@ exports[`test should render Html component with aphrodite styles when possible 1
     <link
       href="test.css"
       rel="stylesheet" />
+    <script
+      dangerouslySetInnerHTML={
+        Object {
+          "__html": "window._aphrodite = [\"stuff\"]);",
+        }
+      }
+      data-react-helmet={true} />
   </head>
   <body>
     <div
@@ -34,13 +41,6 @@ exports[`test should render Html component with aphrodite styles when possible 1
       id="phenomic" />
     <script
       phenomicStuff={true} />
-    <script
-      dangerouslySetInnerHTML={
-        Object {
-          "__html": "window._aphrodite = [\"stuff\"]);",
-        }
-      }
-      data-react-helmet={true} />
     <script
       src="test.js" />
   </body>
diff --git a/src/components/Html/__tests__/__snapshots__/glamor.js.snap b/src/components/Html/__tests__/__snapshots__/glamor.js.snap
index 576ba10f..74533eaf 100644
--- a/src/components/Html/__tests__/__snapshots__/glamor.js.snap
+++ b/src/components/Html/__tests__/__snapshots__/glamor.js.snap
@@ -22,6 +22,13 @@ exports[`test should render Html component with glamor styles when possible 1`]
     <link
       href="test.css"
       rel="stylesheet" />
+    <script
+      dangerouslySetInnerHTML={
+        Object {
+          "__html": "window._glamor = [{\"id\":\"s\"}]",
+        }
+      }
+      data-react-helmet={true} />
   </head>
   <body>
     <div
@@ -33,13 +40,6 @@ exports[`test should render Html component with glamor styles when possible 1`]
       id="phenomic" />
     <script
       phenomicStuff={true} />
-    <script
-      dangerouslySetInnerHTML={
-        Object {
-          "__html": "window._glamor = [{\"id\":\"s\"}]",
-        }
-      }
-      data-react-helmet={true} />
     <script
       src="test.js" />
   </body>
diff --git a/src/static/__tests__/url-as-html.js b/src/static/__tests__/url-as-html.js
index b5f1f0dc..66dd4c5a 100644
--- a/src/static/__tests__/url-as-html.js
+++ b/src/static/__tests__/url-as-html.js
@@ -193,6 +193,7 @@ test("custom script tags", async (t) => {
   <meta charset="utf-8" />
   <meta http-equiv="X-UA-Compatible" content="IE=edge" />
   <meta name="viewport" content="width=device-width, initial-scale=1" />
+  <script data-react-helmet="true" src="http://foo.bar/baz.js"></script>
 </head>
 
 <body>
@@ -221,7 +222,6 @@ test("custom script tags", async (t) => {
       }
     }
   </script>
-  <script data-react-helmet="true" src="http://foo.bar/baz.js"></script>
   <script src="/test.js"></script>
 </body>