livingcss icon indicating copy to clipboard operation
livingcss copied to clipboard

Duplicate resolution of promises within util

Open matmar10 opened this issue 4 years ago • 2 comments

Several areas of the lib/utils.js have duplicate resolution of promises - where an error calls BOTH reject AND resolve:

Example:

  return new Promise((resolve, reject) => {
    fs.readFile(file, 'utf8', function(err, data) {
      if (err) {
        reject(err);
      }

      resolve(data);
    });
  });

This will BOTH reject and resolve the error.

Also, the test cases are ambiguously written. It's not clear what behavior is actually expected:

    it('should handle glob error', function(done) {
      var files = new Error('custom: glob error');

      utils.readFileGlobs(files)
      .then(function() {
        done('readFileGlobs did not throw error');
      })
      .catch(function(err) {
        expect(err.message).to.equal('custom: glob error');
        done();
      });
    });

Because the done() method accepts an error. Fro the Mocha docs:

This callback accepts both an Error instance (or subclass thereof) or a falsy value; anything else is invalid usage and throws an error (usually causing a failed test).

matmar10 avatar Apr 05 '20 06:04 matmar10

@straker If you can comment on the desired functionality and what the test case means to assert, I can fix this. See PR #82 which already fixes based on what is an assumed reasonable functionality.

matmar10 avatar Apr 05 '20 06:04 matmar10

Ya for the longest time I just saw any string passed to done() threw the error with the string as the message, so never bothered to turn it into an actual Error. So any time a done() is called with a string that should be a failed test case and should technically be done(new Error(msg)).

For the code, it should probably be return reject(data) and I forgot the return.

straker avatar Apr 05 '20 18:04 straker