1
\$\begingroup\$

I'm reading an array (of unknown length) of modules package.json files in order to extract the "style" property and store it in an array of its own:

var gulp = require('gulp')
  , path = require('path')
  , Promise = require('bluebird')
  , fs = Promise.promisifyAll(require('fs-extra'));

let components = [
    'comp1',
    'comp2',
    'comp3'
];
let npmScope = '@myscope';
gulp.task('getstyle', () => {
    let componentsBaseLocation = path.join(__dirname, 'node_modules', npmScope);
    Promise.map(components, (componentName) => {
        return fs.readFileAsync(path.join(componentsBaseLocation, componentName, 'package.json'), 'utf8')
    })
    .spread((...packageData) => {
        return packageData.map((packageDatum, i) => {
            packageDatum = JSON.parse(packageDatum);
            tempName = packageDatum.name.match(new RegExp(`${NpmScope}\/(.*?)$`))[1]
            tempStyle = path.join(componentsBaseLocation, tempName, packageDatum.style);
            return {
                name: tempName,
                style: tempStyle
            };
        });
    })
    .then(components => {
        console.log(JSON.stringify(components, null, 2));
    });
});

This works, but I wonder if there are smarter, better, more appropriate ways to use bluebird or promises in general.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ No need to use .spread((...packageData) =>. Can just do .then((packageData) => since the argument for a Promise.map(...).then() handler is already the array you want in your next step. \$\endgroup\$ Commented Dec 7, 2016 at 21:11

1 Answer 1

2
\$\begingroup\$

Several things you could simplify:

  1. You don't need to use .spread() since the data is already in an array.
  2. If you just process each file as it is read, you can avoid the .map() iteration entirely.
  3. You can avoid some temporary variables.
  4. You should declare all local variables with const, let or var as appropriate.

Here's modified code:

var gulp = require('gulp')
  , path = require('path')
  , Promise = require('bluebird')
  , fs = Promise.promisifyAll(require('fs-extra'));

let components = [
    'comp1',
    'comp2',
    'comp3'
];
let npmScope = '@myscope';
gulp.task('getstyle', () => {
    const componentsBaseLocation = path.join(__dirname, 'node_modules', npmScope);
    Promise.map(components, (componentName) => {
        return fs.readFileAsync(path.join(componentsBaseLocation, componentName, 'package.json'), 'utf8').then(data => {
            const packageDatum = JSON.parse(data);
            const tempName = packageDatum.name.match(new RegExp(`${NpmScope}\/(.*?)$`))[1];
            return {
                name: tempName,
                style: path.join(componentsBaseLocation, tempName, packageDatum.style)
            };
        });
    }).then(components => {
        console.log(JSON.stringify(components, null, 2));
    });
});

You may also want to handle a failure in JSON.parse() with a local try/catch handler so rather than end the entire operation, you can handle an individual error and continue with the rest of the processing, but that depends entirely upon how you're using this code and what you want to happen if a parse failure occurs.

You may also want to handle a failure in packageDatum.name.match().

\$\endgroup\$
6
  • \$\begingroup\$ Thanks so much for your answer! I like the elimination of spread. But I think that the two levels that the nested .thens create is very reminiscent of the callback based approach I'm trying to avoid (the then following the fs.readFileAsync and the then following Promise.map. Is that foolish to wish to avoid that? Is a couple layers of then-nesting totally common within the promise ecosystem \$\endgroup\$ Commented Dec 7, 2016 at 22:14
  • \$\begingroup\$ Also I think tempName needs to be left it, because you still use it in the return {name, style} obj. \$\endgroup\$ Commented Dec 7, 2016 at 22:26
  • \$\begingroup\$ @1252748 - There are times when you can avoid nesting with promises and there are times where the code either requires a level of nesting or it makes the code simpler. I thought eliminating the extra .map() iteration of all the results was a simplification. \$\endgroup\$ Commented Dec 7, 2016 at 22:26
  • \$\begingroup\$ @1252748 - Yes, you are correct about tempName. I put it back. \$\endgroup\$ Commented Dec 7, 2016 at 22:27
  • \$\begingroup\$ Yeah I agree. There's a lot of docs out there that laud the flatness the promise chaining paradigm provides. But I know your promise rep on SO is legit so I just wanted to hear you say it definitively :-) Thanks! \$\endgroup\$ Commented Dec 7, 2016 at 22:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.