Quelques essais nodejs rapport à https://n.survol.fr/n/resoudre-le-callback-hell-javascript-102

1- callback.js

import program from "commander";
import glob from "glob";
import path from "path";
import jsonfile from "jsonfile";
import sizeOf from "image-size";


program
  .version("1.0.0")
  .arguments('<manifest> <dir>')
  .action(function(manifest, dir) {
    const source_path = path.join(dir, "**/*.jpg");
    const options = { };
    glob(source_path, options, function (err, files) {
      if (err) {
        console.error("ERROR: %s", err.message);
      } else {
        let flag = file.length;
        let filesMeta = [ ];
        files.map(function (file) {
          sizeOf(file, function(err, dimensions)) {
            if (err) {
              console.error("ERROR: %s", err.message);
            } else {
              dimensions.path = file;
              dimensions.name = path.basename(file);
              filesMeta.push(dimensions);
              flag--;
              if (flag===0) {
                jsonfile.writeFile(manifest, filesMeta, function (err) {
                  if (err) {
                    console.error("ERROR: %s", err.message);
                  } else {
                    console.info("SUCCESS!");
                  }
                });
              }
            }
          });
        });
      }
    });
  }).parse(process.argv);

if (process.argv.length < 3) {
  program.outputHelp();
  exit(1);
}

2- function.js

import program from "commander";
import glob from "glob";
import path from "path";
import jsonfile from "jsonfile";
import sizeOf from "image-size";


program
  .version("1.0.0")
  .arguments('<manifest> <dir>')
  .action(writeFilesDimensions)
  .parse(process.argv);

function writeFilesDimensions(manifest, dir) {
  const source_path = path.join(dir, "**/*.jpg");
  const options = { };
  const next_callback = function (err, files) {
    if (err) {
      console.error("ERROR: %s", err.message);
    } else {
      getFilesDimensions(files, manifest);
    }
  };
  glob(source_path, options, next_callback);
}

function getFilesDimensions(files, manifest) {
  let flag = file.length;
  let filesMeta = [ ];
  files.map(function (file) {
    sizeOf(file, function(err, dimensions)) {
      if (err) {
        console.error("ERROR: %s", err.message);
      } else {
        dimensions.path = file;
        dimensions.name = path.basename(file);
        filesMeta.push(dimensions);
        flag--;
        if (flag===0) {
          writeFile(filesMeta, manifest);
        }
      }
    });
  });
}

function writeFile(filesMeta, manifest) {
  jsonfile.writeFile(manifest, filesMeta, success);
}

function success(err) {
  if (err) {
    console.error("ERROR: %s", err.message);
  } else {
    console.info("SUCCESS!");
  }
}

if (process.argv.length < 3) {
  program.outputHelp();
  exit(1);
}

3- closure.js

import program from "commander";
import glob from "glob";
import path from "path";
import jsonfile from "jsonfile";
import sizeOf from "image-size";


program
  .version("1.0.0")
  .arguments('<manifest> <dir>')
  .action(writeFilesDimensions)
  .parse(process.argv);


function writeFilesDimensions(manifest, dir) {
  let dest_file = manifest;
  let counter = 0;
  let files_meta = [ ];

  const error = function (err) {
    console.error("ERROR: %s", err.message);
  }

  const cb = function(next_cb) {
    return function (err, data) {
      if (err) {
        error(err);
      } else {
        next_cb(data);
      }
    };
  };

  const success = function () {
    console.info("SUCCESS!");
  }

  const writeManifest = function() {
    jsonfile.writeFile(dest_file, files_meta, success);
  };

  const finishAllFilesMeta = function () {
    counter--;
    if (counter === 0) {
      writeManifest();
    }
  };

  const addFileMeta = function (file, dimensions) {
    dimensions.path = file;
    dimensions.name = path.basename(file);
    files_meta.push(dimensions);
    finishAllFilesMeta();
  };

  const getFileDimension = function (file) {
    const next_cb = function (dimensions) { addFileMeta(file, dimensions); };
    sizeOf(file, cb(next_cb) );
  };

  const getFilesDimensions = function(files) {
    counter = files.length;
    files.map( getFileDimension ) ;
  };

  const getFiles = function(dir) {
    
  };
  
  getFiles(dir);
}


if (process.argv.length < 3) {
  program.outputHelp();
  exit(1);
}

4- promise.js

import program from "commander";
import glob from "glob";
import path from "path";
import jsonfile from "jsonfile";
import sizeOf from "image-size";

import promisify from "promisify-node";

const glob_p = promisify(glob, undefined, true);
const write_p = promisify(jsonfile.writeFile, undefined, true);
const size_p = promisify(imageSize, undefined, true);

program
  .version("1.0.0")
  .arguments('<manifest> <dir>')
  .action(writeFilesDimensions)
  .parse(process.argv);

function writeFilesDimensions(manifest, dir) {
  const source_path = path.join(dir, "**/*.jpg");
  const options = { };
  glob_p(source_path, options)
    .then( getFilesDimensions )
    .then( function (data) { writeManifest(manifest, data); } )
    .then( success )
    .catch( error );
}

function error(err) {
  console.error("ERROR: %s", err.message);
}

function sucess() {
  console.info("SUCCESS!");
}

function writeManifest(manifest, data) {
  return write_p(manifest, data);
}

function getFilesDimensions(files) {
  const promises = files.map( getFileDimension ) ;
  return Promise.all( promises );
}

function getFileDimension(file) {
  return size_p(file).then(function (dimensions) { 
    dimensions.path = file;
    dimensions.name = path.basename(file);
    return dimensions;
  });
}


if (process.argv.length < 3) {
  program.outputHelp();
  exit(1);
}

5- async.js

import program from "commander";
import glob from "glob";
import path from "path";
import jsonfile from "jsonfile";
import sizeOf from "image-size";

import promisify from "promisify-node";

const glob_p = promisify(glob, undefined, true);
const write_p = promisify(jsonfile.writeFile, undefined, true);
const size_p = promisify(imageSize, undefined, true);

program
  .version("1.0.0")
  .arguments('<manifest> <dir>')
  .action(writeFilesDimensions)
  .parse(process.argv);

async function writeFilesDimensions(manifest, dir) {
  try {
    const source_path = path.join(dir, "**/*.jpg");
    const options = { };
    const files = await glob_p(source_path, options);
    let files_meta = [ ];
    for (const file of files) {
      let dimensions = await size_p(file);
      dimensions.path = file;
      dimensions.name = path.basename(file);
      files_meta.push(dimensions);
    }
    await write_p(manifest, filesMeta);
    console.info("SUCCESS!");
  } catch(err) {
    console.error("ERROR: %s", err.message);
  }
}

if (process.argv.length < 3) {
  program.outputHelp();
  exit(1);
}

À lire dans l'ordre :

• callback.js
• function.js
• closure.js
• promise.js
• async.js

Note: J'ai recrée mes itérations rapidement après coup uniquement pour pouvoir discuter de la forme. Elles n'ont pas été débogguées et il est peu probable qu'elles tournent telles quelles.

De même, je sais que glob, sizeOf et writeFile ont toutes les trois une version synchrone. Dans mon exemple de CLI, cette version synchrone est probablement même la plus pertinente. Mon objectif ici était de tester et apprendre à partir d'un petit script plutôt que de me prendre les murs plus tard avec un contexte plus lourd. Utiliser les versions asynchrones est donc une façon de me confronter au problème, pas la source du problème lui-même.

De même le style et le nom des fonctions n'ont pas été travaillées et ne m'intéressent pas dans le contexte de ces essais. Si je fais quelque chose qui n'est "pas dans l'esprit JS" ça m'intéresse, mais je préfère que vous me le remontiez ailleurs qu'ici si ça n'est pas lié aux questions callback/promesses/async.

Callback.js

C'est ma première approche. J'ai une suite difficile à lire, une gestion d'erreur à chaque imbrication, et bien trop d'imbrications pour ma santé mentale.

Function.js

J'ai tenté de séparer mon code en différentes fonctions nommées. C'est un échec total. Je ne suis même pas convaincu que ce soit plus lisible que la première version.

Déjà chaque fonction est dédiée à mon programme parce que la fonction suivante à exécuter est en dur dans le code. Je garde aussi des imbrications et le code de getFilesDimensions est franchement moche : j'ai du mettre un compteur pour exécuter un callback sur toutes les entrées d'un tableau et continuer uniquement après que le dernier soit fini.

Closure.js

J'ai tenté de simplifier mes fonctions en les découpant plus. Pour ça j'ai du créer des closures. Elles me servent à avoir des sortes de variables globales à ma chaîne de callback. Pas top top mais je voulais essayer.

Mes fonctions sont effectivement mieux découpées mais trop. Suivre le programme demande de suivre une à une chaque fonction et difficile de savoir où on va. Chaque partie est simple, mais le programme est complexe parce qu'on manque de vue d'ensemble.

Promise.js

J'enlève les callback pour passer avec des promesses via Promisify. Ça commence à être lisible en suivant l'enchaînement des then mais j'ai encore un truc moche ligne 24. Les promesses ne remontent que les données résultat de l'étape précédente. Si je veux utiliser une données de plusieurs étapes en amont, il me faut une closure quelque part (ou alors je n'ai pas vu comment faire).

La gestion d'erreur est aussi bien plus simple avec le catch final.

Async.js

Je passe par async/await. Franchement c'est de loin ce que je considère comme plus lisible pour un petit script comme ça.

Seul défaut : Pour éviter le Promise.all j'ai choisi de passer par une bête boucle. Ça m'évite aussi une closure (oui, quand je peux je préfère éviter). Sauf erreur ça veut dire que je récupère les tailles des images une à une au lieu de le faire en parallèle. Le ratio lisibilité/performance dépendra de l'usage. Là j'ai préféré la lisibilité. Au pire on reprend le Promise.all de la version précédente.

Bon, cela dit c'est smart, mais je suis encore gêné de devoir gérer les conséquences de l'event loop moi-même en complexifiant mon code, au lieu que ce soit le langage et son runtime qui gèrent ces questions pour moi.

Tout ça pour ça… Et bé, ça donne pas envie JS...

Je passe sur les 3 premiers fichiers, tu as tout dis. En fait, tu as un peu revécu l'histoire des pratiques JS sur l'asynchrone. Aujourd'hui on en est aux promesses et demain async/await.

Sur le fichier Promise.js:

• Je sais pas si c'est moche ligne 24, mais c'est la manière la plus simple.
• Ton programme est faux pour plusieurs raisons qui ont la même cause : aux lignes 24, 38 et 27, il manque des return. Ca a pour effet de retourner des undefined directement avec 2 conséquences facheuses. 1) le callback du .then suivant est appelé plus tôt que prévu (avant que l'opération asynchrone ne soit achevée) 2) tu rates les erreurs s'il y en a.
  C'est un peu pourri, mais c'est comme ça. Dans un monde idéal un linter basé sur TypeScript ou flow pourrait imposer de toujours retourner/gérer les promesses générées par un appel de fonction, mais ça n'existe pas à ma connaissance malheureusement.
• es6-promisify a l'air moins verbeux. Aussi, souvent les packages ont leur version promise https://www.npmjs.com/package/glob-promise
• Y'a plein d'occasions d'utiliser des arrow functions mais c'est une question d'habitude et de goût.

Mais c'est à peu près le code que j'aurais écris, donc j'ai pas grand chose à redire (même si je sais que ça prend du temps avant d'en arriver à là :-p)

Je ne maitrise pas async/await, donc je ne sais pas le commenter, mais je crois que le choix de mettre un await à l'intérieur de la boucle change les caractéristiques de performance (les fichiers sont traités séquentiellement, alors qu'ils l'étaient concurrenciellement avec Promise.all)

Pour la "ligne 24", tu peux peut-être utiliser le Currying, une pratique de programmation fonctionnelle qui te permet de transformer une fonction prenant plusieurs paramètres en un ensemble de fonctions préparées. Tu aurais ainsi à disposition une fonction writeManifestBis qui ne prendrait que data comme param.

En complément du commentaire

async function write_file_dimensions(manifest, dir) {
    const source_path = path.join(dir, "**/*.jpg");
    const options = { };

    try {
        filepaths = await glob_p(source_path, options);
        file_metadatas = await Promise.all(filepaths.map(get_metadata))
        write_p(manifest, file_metadatas)
    } catch(err) {
        console.error(`ERROR: ${err.message}`);
    }
}

async function get_metadata(filepath) {
    const dimensions = await size_p(filepath);
    return {
        ...dimensions,
        path: filepath,
        name: path.basename(filepath),
    }
}

Il manque sans doute une solution à base de générateurs, qui ressemblerait fortement à celle basée sur async/await; la nuance principale résiderait dans l'écriture des tests, où les valeurs récupérées par yield à chaque itération se mockent plus facilement.

@DavidBruant: Corrigé pour les valeurs de retour, merci. Je suis trop habitué aux facilités de Ruby.
Effectivement aussi pour le côté séquentiel de la boucle par rapport au parallélisme de Promise.all

@borisschapira: Oui, j'ai hésité pour la fonction qui génère une fonction, d'autant que j'avais commencé un pas dans cette direction avec cb sur la version closures. Maintenant j'ai trouvé que c'était déjà assez complexe à suivre ainsi. Une closure m'apparaissait finalement aussi simple si c'est pour l'utiliser une seule fois.

@rik: C'est vrai que ta solution pour retourner l'objet contenant nom de fichier et dimensions est vachement plus classe que la mienne. Modifier l'objet dimension directement n'était pas super élégant.

Avant toute chose, merci pour ce travail de "mise au propre" de tes recherches en JavaScript. J'ai découvert des choses intéressantes (pratiques de CLI en JS notamment) ! J'espère que je vais pouvoir à mon tour t'apporter quelques éléments.

De mon côté j'aurai pris l'approche Promise sans hésiter. J'ai repris ta version et apporté quelques modifications :
https://gist.github.com/Lythom/d49e1fa67d2a4cb14b79e42554b05c68#file-4-promise-js

Liste des changements :

• Suppression de promisify et utilisation directe de l'objet Promise.
  • Le code de la lib est difficile à lire et plutôt complexe : en cas de problème le debug est plus facile sans passer par la lib
  • L'utilisation directe et explicite des Promise est facile à lire et à comprendre, là où le fonctionnement de la fonction de haut niveau nécessite de se documenter sur la librairie. (réduire le nombre d'indirections facilite la lecture du code de manière générale)
• Pour glob, utilisation directe du callback plutôt que de passe par une promise.
  • Sans promisify encapsuler le tout sous une Promise juste pour gérer le flux d'erreur est plus complexe que d'avoir une ligne pour le cas d'erreur
• Utilisation des arrow fonctions es6 dans la chaine de "then"
  • Question de préférence, mais je trouve qu'expliciter à chaque ligne l'entrée utilisée par le traitement permet d'appréhender plus facilement l'ensemble du traitement. A chaque ligne, la variable (à nommer explicitement !) est à la fois l'output de la fonction du dessus et l'input de la fonction adjacente.
• Inlining de la fonction getFilesDimensions.
  • La fonction fait peu de chose et ça permet d'expliciter l'utilisation de la Promise comme entrée de la chaîne de traitement.
• Utilisation de require à la place de import
  • Et on peut alors se passer de babel complètement =)
• Je n'ai pas ajouté de commentaires de code pour pouvoir comparer la lisibilité du code seul, mais en vrai sur un livrable, j'aurai mis quelques indications sur les lignes les moins évidentes pour synthétiser l'action réalisée.

Autres commentaires sur les discussions :

• L'utilisation de la closure pour faire descendre le manifest en paramètre est une bonne approche car les 2 variables utilisées par la fonctions sont ainsi faciles à tracer : on ajoute pas d'indirection. Avec la chaîne de traitement en style "arrow function" ça n'est d'ailleurs plus choquant du tout.
• Je n'ai pas l'habitude d'utiliser async/await, pour le moment j'évite pour des raisons de debug : le code est nécessairement transpilé et je sais pas si j'ai très envie d'avoir à mettre le nez dedans.
• D'une manière générale les appels asynchrones ne sont pas si fréquents (pour le web en tout cas, évidemment ça dépend toujours de ce que fait l'application), et ceux qui sont fréquents (fetch d'API, d'urls) retournent des Promise directement.