Prevent stylesheets from adding twice (or more!) #4

adammcarth · Jun 13, 2014

The problem:

This fix removes the existing stylesheet (by checking the href parameter against the one passed to the loadCSS() function) before the new one is added to the DOM.

Adam :)

scottjehl · Jun 13, 2014

Hey, thanks. Can you explain the issue this change addresses? I'm not sure why you're trying to load the same file twice.

adammcarth · Jun 14, 2014

Sure

So let's say you have a form that submits though ajax and you want to load a new stylesheet asynchronously if the server hits back with a 200 OK (perhaps some css to format a success message). If the form is submitted twice or more (very feasible in some scenarios) - the stylesheet will be appended to the DOM more than once. Although this shouldn't make a difference to the rendering of the css, appending the same stylesheet to the document more than once seems very wrong.

So no, you'd be out of your mind to add the stylesheet twice like I did in my example above - I just wanted to show that it will be added again without removing the old one first.

The changes I made check each stylesheet's href="" attribute, and if it matches the one specified in the loadCSS() function - it will be removed before it is added again. I was doing some deeper thinking whilst running this morning, perhaps it should check that the media="" attribute is the same too before removing it?

adammcarth · Jun 14, 2014

Checking the media attribute would get very messy since ss.media = media || "all" was used in a setTimeout() function. You'd have to set something like data-real-media to access the real media value (instead of "only x"), but I don't want to overcomplicate things at this stage. If we were to do that I think it should be a separate PR.

adammcarth · Jun 23, 2014

How are we feeling @scottjehl?

scottjehl · Jun 23, 2014

Thanks for checking in, @adammcarth. We talked it over internally and we don’t feel that this addition meets a common enough use case to include in this simple script. That said, I’d encourage you to keep the fork open if you find it useful! Thanks again for the PR.

Benjamin-K · Jun 24, 2014

Why not just cancelling the second request/change?

inta · Jun 24, 2014

Great point Benjamin. I think it would be better not to reload already loaded files, that would result in unnecessary HTTP requests.

adammcarth · Jun 24, 2014

@Benjamin-K @inta I thought for quite a long time about that. Reload the stylesheet or prevent it from adding again?

I settled on reloading it - what if the stylesheet was dynamically modified by an external process? The changes wouldn't be applied if we ignored the request. Whilst most adding it again would probably result in an unnecessary http request most of the time, you have to cater for all scenarios.

inta · Jun 24, 2014

Yeah, I thought about that, but came to the conclusion that this is a rare use case. In production environments I do not change my stylesheets anyway, as they should be cached. I think this is out of the scope of this script, because you need to clear the cache either if you really want to force reload of a CSS file.

adammcarth · Jun 24, 2014

True. I'll probably update my branch with that functionality in that case - but I still don't think it will change Scott's mind.

scottjehl closed this Jun 23, 2014

adammcarth	`prevent stylesheet from adding twice` … - at the moment if loadCSS() is called twice (this situation is perfectly feasible), the stylesheet will also be added twice. This fix removes the existing stylesheet (by checking the href parameter against the one passed to the loadCSS() function) before the new one is added to the DOM.	`a554ace`
adammcarth	`update minified version for new patch`	`4d3db6b`
adammcarth	`fix the formatting` … 2 spaces out please	`ed9983a`
adammcarth	`fix formatting v2`	`8efb010`

@@ @@ -4,24 +4,34 @@ loadCSS: load a CSS file asynchronously. @@
  Licensed MIT
  */
  function loadCSS( href, before, media ){
--	"use strict";
--	// Arguments explained:
--	// `href` is the URL for your CSS file.
--	// `before` optionally defines the element we'll use as a reference for injecting our <link>
--	// By default, `before` uses the first <script> element in the page.
--	// However, since the order in which stylesheets are referenced matters, you might need a more specific location in your document.
--	// If so, pass a different reference element to the `before` argument and it'll insert before that instead
--	// note: `insertBefore` is used instead of `appendChild`, for safety re: http://www.paulirish.com/2011/surefire-dom-element-insertion/
--	var ss = window.document.createElement( "link" );
--	var ref = before || window.document.getElementsByTagName( "script" )[ 0 ];
--	ss.rel = "stylesheet";
--	ss.href = href;
--	// temporarily, set media to something non-matching to ensure it'll fetch without blocking render
--	ss.media = "only x";
--	// inject link
--	ref.parentNode.insertBefore( ss, ref );
--	// set media back to `all` so that the styleshet applies once it loads
--	setTimeout( function(){
--		ss.media = media || "all";
--	} );
-- }
++  "use strict";
++  // Arguments explained:
++  // `href` is the URL for your CSS file.
++  // `before` optionally defines the element we'll use as a reference for injecting our <link>
++  // By default, `before` uses the first <script> element in the page.
++  // However, since the order in which stylesheets are referenced matters, you might need a more specific location in your document.
++  // If so, pass a different reference element to the `before` argument and it'll insert before that instead
++  // note: `insertBefore` is used instead of `appendChild`, for safety re: http://www.paulirish.com/2011/surefire-dom-element-insertion/
++
++  // DELETE OLD STYLESHEET IF IT ALREADY EXISTS
++  var current_ss = window.document.getElementsByTagName( "link" );
++  for ( var i = 0, ss; ss = current_ss[i]; i++ ) {
++    if ( ss.getAttribute( "href" ) === href ) {
++      ss.remove();
++    }
++  }
++
++  // ADD THE NEW STYLESHEET
++  var ss = window.document.createElement( "link" );
++  var ref = before || window.document.getElementsByTagName( "script" )[ 0 ];
++  ss.rel = "stylesheet";
++  ss.href = href;
++  // temporarily, set media to something non-matching to ensure it'll fetch without blocking render
++  ss.media = "only x";
++  // inject link
++  ref.parentNode.insertBefore( ss, ref );
++  // set media back to `all` so that the styleshet applies once it loads
++  setTimeout( function(){
++    ss.media = media || "all";
++  } );
++}

filamentgroup/loadCSS

HTTPS clone URL

Subversion checkout URL

Prevent stylesheets from adding twice (or more!) #4

Labels

Milestone

Assignee

4 participants