LinkedInAttic / inject

AMD and CJS dependency management in the browser
http://www.injectjs.com
Other
464 stars 48 forks source link

Two Bug Fixes #307

Closed gvsboy closed 10 years ago

gvsboy commented 10 years ago

This pull request addresses two issues affecting IE:

  1. When attaching multiple stylesheets using the css plugin, only the last sheet will be applied in IE <= 10 as each sheet clobbers the sheet before it. The problem is using innerHTML on dynamically created style elements (which is what the plugin does for browsers--basically IE--identified by feature sniffing the styleSheet object on style elements). Mainly, innerHTML tends to return an empty string regardless of the content, so when multiple stylesheets are attached only the last one is available (as each preceding sheet clobbers one another). The provided solution is to cache the stylesheet content for subsequent fetches can concat.

Also, after discussing with @eoneill, a few more updates in a future patch will guard against other issues involving charset and media declarations, and overall selector limit.

  1. The removeGlobalObject method in executor deletes a window property, which IE8 doesn't like. I've added a try/catch fallback which is setting the property to undefined. This makes IE8 happy.

I'm getting this error in Travis: https://travis-ci.org/gvsboy/inject/builds/31279503 Do you know what I need to do to remedy? Some configuration I'm missing?

eoneill commented 10 years ago

To fix the travis-ci failure, add quotes around the node_js version in .travis.yml per travis-ci/travis-ci#2591

e.g.

language: node_js
node_js:
  - "0.10"
...
eoneill commented 10 years ago

For record keeping, here's an approach to measure the number of selectors in a stylesheet to avoid hitting the 4095 limit (note that this isn't tested at all):

function selectorCountForStylesheet(stylesheet) {
  var selectors = 0;
  var len;
  try {
    if (stylesheet && stylesheet.cssRules) {
      len = stylesheet.cssRules.length;
      while (len--) {
        if (stylesheet.cssRules[j].selectorText) {
          selectors += stylesheet.cssRules[j].selectorText.split(',').length;
        }
      }
    }
  }
  catch(e) {}

  return selectors;
};

...

// you would have to do this check AFTER you append to the existing style node...
// note that for most other browsers, if you exceed this limit, we're fine, so we don't want to catch the `greater than` case
//  if you have exactly 4095 selectors in your stylesheet by chance, you should by a lottery ticket
if (selectorCountForStylesheet(style) === 4095) {
  // reached the selector limit, create a new stylesheet and assign it to `style`, then retry
}
gvsboy commented 10 years ago

Awesome. Thanks @eoneill for documenting your selector counting approach and the Travis fix. All systems go!

jakobo commented 10 years ago

Looks good @gvsboy and @eoneill. Thanks for helping find and quash this.