Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Prevent duplicate loading of JS files #904

Merged
merged 3 commits into from
Dec 16, 2016
Merged

Prevent duplicate loading of JS files #904

merged 3 commits into from
Dec 16, 2016

Conversation

timothycoy
Copy link
Contributor

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.

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
Copy link
Owner

MoOx commented Dec 3, 2016

Hi. Is this really creating an issue? Can you give more details about this?

@timothycoy
Copy link
Contributor Author

timothycoy commented Dec 3, 2016

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

Here you can see polyfil and google maps are being loaded twice once in the head and once in the body.
Screenshots:
screen shot 2016-12-03 at 10 02 55 am
screen shot 2016-12-03 at 10 03 10 am

@MoOx
Copy link
Owner

MoOx commented Dec 8, 2016

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
Copy link
Contributor Author

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
Copy link
Contributor

arlair commented Dec 13, 2016

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 as far as I could tell. headElement being set here.

@MoOx
Copy link
Owner

MoOx commented Dec 14, 2016

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

@timothycoy
Copy link
Contributor Author

Done.

@MoOx
Copy link
Owner

MoOx commented Dec 16, 2016

Thanks! I will release this as a minor (not a patch) as some people might get unexpected results.

@MoOx MoOx merged commit 23a9ad7 into MoOx:master Dec 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants