$css.preload may call the callback function too early

[kayhadrin]

Hi there,

I was reviewing this promising angular module and found a small algorithm issue in the $css.preload() function.

Here's the original function code:

/**
 * Preload: preloads css via http request
 **/
$css.preload = function (stylesheets, callback) {
  // If no stylesheets provided, then preload all
  if (!stylesheets) {
    stylesheets = [];
    // Add all stylesheets from custom directives to array
    if ($directives.length) Array.prototype.push.apply(stylesheets, $directives);
    // Add all stylesheets from ngRoute to array
    if ($injector.has('$route')) Array.prototype.push.apply(stylesheets, $css.getFromRoutes($injector.get('$route').routes));
    // Add all stylesheets from UI Router to array
    if ($injector.has('$state')) Array.prototype.push.apply(stylesheets, $css.getFromStates($injector.get('$state').get()));
  }
  stylesheets = filterBy(stylesheets, 'preload');
  angular.forEach(stylesheets, function(stylesheet, index) {
    // Preload via ajax request
    $http.get(stylesheet.href)
      .success(function (response) {
        $log.debug('preload response: ' + response);
        if (stylesheets.length === (index + 1) && angular.isFunction(callback)) 
          callback(stylesheets);
      })
      .error(function (response) {
        $log.error('Incorrect path for ' + stylesheet.href);
      });
  });
};

I can see that angular.forEach(stylesheets, ... will make the program download each stylesheet by ajax, and for each stylesheet download success, we'll run the callback function only when we downloaded the last stylesheet in the list.

For me, the problem is that we assumed that stylesheets will be loaded in sequential order. However, due to the nature of ajax requests and the unpredictable response speed of servers, this cannot be guaranteed.

So I think this part of the algorithm should take advantage of promises to make sure that the callback function is called at the end when all stylesheets have been loaded successfully.

So here's my suggested code change:

// Assuming that $q is injected in the module's JS scope

var stylesheetLoadPromises = [];
angular.forEach(stylesheets, function(stylesheet) {
  stylesheetLoadPromises.push(
    // Preload via ajax request
    $http.get(stylesheet.href)
      .success(function (response) {
        $log.debug('preload response: ' + response);
        if (stylesheets.length === (index + 1) && angular.isFunction(callback)) 
          callback(stylesheets);
      })
      .error(function (response) {
        $log.error('Incorrect path for ' + stylesheet.href);
      })
  );
});

if (angular.isFunction(callback)) {
  $q.all(stylesheetLoadPromises).then(function(){
    callback(stylesheets);
  });
}

Please let me know what you think.

Regards,

David

[alexcastillo]

Hi David,

I completely agree with you. This is a great solution and I'm sure it will make the preload feature better.

Do you mind submitting a pull request with these changes? I'll be happy to merge it.

Thank you for your contribution!

Alex Castillo

[alexcastillo]

Hi @kayhadrin

I've pushed a fix based on your suggestions. Please review.

Best,

Alex

[kayhadrin]

Thanks @alexandercastillo. It looks fine to me.

[kayhadrin]

Just wondering why you use the parse(stylesheet) instruction now? Is it because stylesheet could be a string representation of an angular hash object?

[alexcastillo]

@kayhadrin I use parse(stylesheet) so the stylesheets argument in the callback contain the rest of the properties AngularCSS is adding internally. I think that could be relevant to the developer.