[Critique] Loader dynamique de script jQuery

Version imprimable

Bonjour à tous, j'aimerais recevoir les critiques d'une fonction que j'utilise de temps en temps. En fait je ne sais pas si il est intéressant de m'en servir, si elle respecte les bonnes pratiques de développement ou si tout simplement elle est bien développé mais en tout cas je tiens à préciser que j'ai pris plaisir à la créer :)

Il s'agit simplement d'un loader de script qui permet de charger des scripts (obvious) dans le bon ordre (c'est la ou je voulais en venir :)). Bref voici le code et l'utilisation.
loader.js:

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
 
var loadScript = (function ($) {
  if ($) {
    jQuery.loadScript = function (url, options) {
      options = $.extend(options || {}, {
        dataType: "script",
        async: false,
        url: url
      });
 
      return jQuery.ajax(options);
    };
 
    return function (urls, path) {
      var queue = urls.map(function (script) {
          return $.loadScript((typeof path !== 'string' ? '' : path) + script + '.js');
      });
 
      $.when.apply(null, queue)
        .done(function () {
          var strUrl = '';
 
          for (var i = 0; i < arguments.length; i++) {
            strUrl += ' "' + urls[i] + '"' + (i !== arguments.length-1 ? ',' : '');
          }
 
          console.log('%c' + strUrl + (urls.length > 1 ? ' have' : ' has') + ' been loaded', 'color: blue;');
        })
 
        .fail(function (a, b, c) {
          for (var i = 0; i < urls.length; i++) {
            if (this.url.split('?')[0] === urls[i]) {
              throw new Error('failed to load ' + urls[i] + ', Message "' + b + ': ' + c + '"');
            }
          }
        });
    }
  }
}($));

appel.html

Code:

1
2
3
4
5
6
7
8
 
<script src="js/loader.js"></script>
<script type="text/javascript">
  loadScript(
    ['leaflet-src', 'NonTiledLayer', 'NonTiledLayer.WMS', 'google', 'tooltip', 'map.handler', 'local', 'utils', 'intersects', 'leaflet.contextmenu'], 
    'js/map/'
  );
</script>

05/03/2015, 10h49
Watilin
Code:

async: false,

Là tu fais un appel Ajax synchrone donc. Qui bloque le thread UI. C'est voulu ?
Tout le monde n'a pas mon opinion mais pour moi on ne bloque le thread UI que quand on a besoin de bloquer le thread UI. Le cas d'école, c'est quand on veut faire une opération de dernière minute (typiquement une sauvegarde) avant la fermeture de la page, sur l'évènement unload (lire à ce sujet : Don't let the door hit you…). Toute autre forme d'Ajax synchrone est à éviter, mais je le répète, ce n'est que mon humble avis. Tant que tu construis ta page selon le principe d'amélioration progressive, je ne vois pas d'inconvénient à laisser l'utilisateur utiliser la page avant que les scripts aient tous fini de charger.

Du coup si tu touches à cet aspect synchrone, il faudra revoir la façon dont les urls sont mappés dans queue car, en l'état, elles sont toutes mappés en même temps donc les chargements sont tous initiés en même temps.

Autre chose, j'aurais passé jQuery plutôt que $ à ma fonction wrapper, comme ceci :
Code:

1 2 3 4 5 var loadScript = (function ($) { ... }(jQuery));
Tu déclares loadScript dans l'espace global. Pourquoi ?

Et sinon, j'aurais pris la peine de donner des noms un peu plus explicites aux arguments a, b et c.

Le reste me paraît élégant. C'est toujours agréable de travailler avec les promises :)

Sur la question « s'il est intéressant de s'en servir », oui bien sûr, son usage est légitime. En gros elle fonctionne comme l'attribut defer, qui existe depuis IE4, en assurant un comportement correct là où les navigateurs ne respectent pas toujours la spec (entre autres IE8 et Safari 4 – détails : The script defer attribute).

Edit: En fait, il semblerait que la donne n'ait pas beaucoup changé depuis 2007… :roll:
05/03/2015, 13h48
Darkaurora

Citation:

Envoyé par Watilin

Là tu fais un appel Ajax synchrone donc. Qui bloque le thread UI. C'est voulu ?

Oui c'est voulu en fait il existe une partie des sources qui utilise les scripts chargés par loadScript. Sans cette synchronisation le reste des scripts plantent. Pour palier a cet inconvénient (car je suis d'accord avec toi) il faudrait peut être un callback ou un autre procédé qui temporise.

Citation:

Envoyé par Watilin

Tu déclares loadScript dans l'espace global. Pourquoi ?

Tout simplement pour utiliser la fonction par la suite, je ne sais pas comment faire autrement :)

Sinon merci pour ces remarques, c'est en partie grâce à vous que j'ai appris ce que je sais du JS et je n'ai pas envie de m'arrêter la :)

Citation:

Envoyé par Darkaurora

Oui c'est voulu en fait il existe une partie des sources qui utilise les scripts chargés par loadScript. Sans cette synchronisation le reste des scripts plantent. Pour palier a cet inconvénient (car je suis d'accord avec toi) il faudrait peut être un callback ou un autre procédé qui temporise.

Vite fait je dirais une fonction récursive, enfin indirectement récursive, puisqu'elle s'auto-ajouterait en callback de l'évènement load du prochain script, une fonction asynchroniquement récursive si tu vois ce que je veux dire…
Code:

1 2 3 4 5 6 7 8 9 10 var queue = [ url1, url2, ... ]; (function loadNextScript() { if (queue.length) { var newScript = document.createElement("script"); newScript.src = queue.shift(); newScript.onload = loadNextScript; // récursivité document.body.appendChild(newScript); } }()); // appel immédiat
Bon j'ai pas testé ce bout de code, mais tu saisis l'idée. Grâce à shift qui fait sortir les éléments par la gauche du tableau, on respecte l'ordre des urls, et on est sûr qu'un script ne commence pas à charger avant que le précédent ait terminé. Après pour être complet il faudrait gérer les erreurs, et là ça devient intéressant de le faire avec des promises. Hmm, je vais me pencher dessus…

Citation:

Tout simplement pour utiliser la fonction par la suite, je ne sais pas comment faire autrement :)

Étant donné que tu l'ajoutes à l'espace de noms jQuery, tu as déjà une manière d'y accéder par $.loadScript. À propos, en relisant ton code, je ne suis pas sûr de comprendre les endroits où tu alternes $ et jQuery. Par exemple ici :
Code:

1 2 if ($) { jQuery.loadScript = ...

C'est vrai qu'en le relisant tout à l'heure je me suis fait la même remarque que j'ai corrigé tout de suite après d'ailleurs. Cependant la fonction jQuery.loadScript et loadScript ne sont pas du tout les mêmes fonctions en fait j'utilise jQuery.loadScript dans loadScript :)

Pour ce qui est de la récursivité ça ne suffira pas, certes les scripts vont se charger les uns à la suite des autres mais pour l'exemple suivant je soulève une erreur qui interrompt l’exécution du main script:
Code:

1 2 3 4 5 6 7 8 9 <script src="js/loader.js"></script> <script type="text/javascript"> loadScript( ['leaflet-src', 'NonTiledLayer', 'NonTiledLayer.WMS', 'google', 'tooltip', 'map.handler', 'local', 'utils', 'intersects', 'leaflet.contextmenu'], 'js/map/' ); var map = L.map('map').setView([51.505, -0.09], 13); </script>
La lib leaflet n'étant pas chargé une erreur est levé et ça plante. Je pensais plus à quelque chose de cette manière:
Code:

1 2 3 4 5 6 7 8 9 <script src="js/loader.js"></script> <script type="text/javascript"> loadScript( ['leaflet-src', 'NonTiledLayer', 'NonTiledLayer.WMS', 'google', 'tooltip', 'map.handler', 'local', 'utils', 'intersects', 'leaflet.contextmenu'], 'js/map/', function () { var map = L.map('map').setView([51.505, -0.09], 13); }); </script>
Le problème c'est que ça serait très moche avec un code conséquent. Grosso modo l'utilité d'une telle fonction (de mon point de vue) est de chargé facilement toutes les dépendances ou plugin d'une classe avant de les utiliser.

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
<script>
    ( function loadScript( $, queue ){
        if ( queue && queue.length ){
            var jqXHR = $.getScript( queue.shift() )
 
            setTimeout( function(){
                // Si l'objet différé existe dans l'état "attendre", 
                // alors exécute la méthode fail()
 
                if ( jqXHR && jqXHR.state() === "pending" ){
                jqXHR.abort();
                }
            }, 3000 ); // 3s
 
            jqXHR.done( function( data, textStatus, jqXHR ){
                // succès de la transaction, on doit traiter le contenu de data
                console.log( data, textStatus, jqXHR );
 
            });
 
            jqXHR.fail( function( jqXHR, textStatus, errorThrown ){
                // échec de la transaction, gérer la catastrophe
                console.log( jqXHR, textStatus, errorThrown );
 
            });
 
            jqXHR.always( function( jqXHR, textStatus ){
                // la transaction est terminée
                // nettoyage
                // exécution d'un code dépendant
                //console.log( jqXHR, textStatus );
 
                loadScript( $, queue );
            });
        }
    }( jQuery, [ "config.js", "data.js" ] ));
</script>

06/03/2015, 13h19
Watilin

Citation:

Envoyé par Darkaurora

Cependant la fonction jQuery.loadScript et loadScript ne sont pas du tout les mêmes fonctions en fait j'utilise jQuery.loadScript dans loadScript :)

En effet. J'ai fait une bourde :oops: Mais alors, tu as deux fonctions, loadScript et $.loadScript, qui sont toutes les deux accessibles depuis l'extérieur et qui ne font pas la même chose. Ça me dérange quand même un petit peu.

@Daniel : c'est une réécriture complète que tu nous proposes là ;)
06/03/2015, 13h25
danielhagnoul

Si on utilise jQuery, autant utiliser les méthodes disponibles. C'est plus simple et plus sûr.

Citation:

Envoyé par Watilin

En effet. J'ai fait une bourde :oops: Mais alors, tu as deux fonctions, loadScript et $.loadScript, qui sont toutes les deux accessibles depuis l'extérieur et qui ne font pas la même chose. Ça me dérange quand même un petit peu.

Tout à fait d'accord et quand hier nous parlions de l'amalgame entre $ et jQuery je me suis rendu compte également de cette bourde:
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 var loadScript = (function ($) { if ($) { var loadScript = function (url, options) { options = $.extend(options || {}, { dataType: "script", async: false, url: url }); return jQuery.ajax(options); }; return function (urls, path) { var queue = urls.map(function (script) { return loadScript((typeof path !== 'string' ? '' : path) + script + '.js'); }); $.when.apply(null, queue) .done(function () { var strUrl = ''; for (var i = 0; i < arguments.length; i++) { strUrl += ' "' + urls[i] + '"' + (i !== arguments.length-1 ? ',' : ''); } console.log('%c' + strUrl + (urls.length > 1 ? ' have' : ' has') + ' been loaded', 'color: blue;'); }) .fail(function (jqXHR, status, error) { for (var i = 0; i < urls.length; i++) { if (this.url.split('?')[0] === urls[i]) { throw new Error('failed to load ' + urls[i] + ', Message "' + status + ': ' + error + '"'); } } }); } } }(jQuery));
Citation:

Envoyé par danielhagnoul

Si on utilise jQuery, autant utiliser les méthodes disponibles. C'est plus simple et plus sûr.

C'est vrai ta méthode est d'un simplicité déconcertante :) cependant, et ça n'engage que moi, je n'aime pas utiliser un timeout surtout dans ce cas ci, c'est d'ailleurs ce qui m'a poussé à utiliser des promises et des deffered et en plus ça me permet de me familiariser un peu mieux avec ces concepts.

06/03/2015, 14h19
danielhagnoul

Le Timeout est seulement là pour refermer la transaction AJAX dans le cas très rare où la transaction reste en "stand-by" trop longtemps. Voir :

http://danielhagnoul.developpez.com/...AjaxAbort.html