medikoo / deferred

Modular and fast Promises implementation for JavaScript
ISC License
364 stars 20 forks source link

Reject method #3

Closed zemd closed 12 years ago

zemd commented 12 years ago

I am trying to find file recursively from current folder to top folders, until some limit. I need to compare paths in series, so if I find file in current folder - I resolve with success.

// Helper function:
  function pathExists(path) {
    var result = deferred();
    PATH.exists(path, function (exists) {
      if (exists) {
        result.resolve(path);
      } else {
        result.resolve(new Error('No such path: ' + path));
      }
    });
    return result.promise;
  }

function findFile(filename, basepath) {
    /*jshint eqnull:true*/
    if (null == basepath) {
      basepath = process.cwd();
    }
    var fDepth = FOLDER_DEPTH,
      folderPath = basepath,
      basePaths = [],
      findFileDeferred = deferred();

    // tricky function
    var promiseExists = function (filePath) {
      return function () {
        var d = deferred();
        pathExists(filePath)
          .then(function () {
            d.resolve(new Error(filePath)); // <-------- (1)
          }, function () {
            d.resolve(filePath);
          });
        return d.promise;
      };
    };

   // find all paths where to search file
    while (fDepth-- && !myutil.isPathRoot(folderPath)) {
      basePaths.push(promiseExists(PATH.join(folderPath, filename)));
      folderPath = PATH.join(folderPath, '..');
    }

    deferred.reduce(basePaths, function (previousValue, currentValue, index, array) {
      return deferred(currentValue());
    }, basepath[0])
      .then(function () {
        findFileDeferred.resolve(new Error("No such file: " + filename));
      },
      function (error) {
        findFileDeferred.resolve(error.message); // <----- (2) 
      });
    return findFileDeferred.promise;
  }

(1) - I think there should not be Error. I reject result with some params, but it is not Error. There should be behavior like Error, but without Error.

(2) - here I need filePath that exists, and it is not good to take it from Error message

ps Also I think all this function is not so well for usage, and if you can - please point me how to make right, and sorry for my english.

medikoo commented 12 years ago

@hunter it's great use case.

So, firstly I would use path.exists function as it is Node, so it always succeeds with either true or false:

var pathExists = deferred.promisify(require('path').exists);

Secondly, I think best solution for that scenario would be to do recursive call of a function that stops if path exists and runs itself with deeper path in other case. Promises can be resolved with other promises and that's why it works well. See:

function findFile(filename, basepath) {
    /*jshint eqnull:true*/
    if (null == basepath) {
      basepath = process.cwd();
    }
    var fDepth = FOLDER_DEPTH,
      folderPath = basepath;

    return (function self(fDepth, folderPath) {
        var filePath;
        if (fDepth && !myutil.isPathRoot(folderPath)) {
            filePath = PATH.join(folderPath, filename);
            return pathExists(filePath)
                .then(function (exists) {
                    // If exists resolve with filePath, if not go deeper
                    return exists ? filePath :
                        self(--fDepth, PATH.join(folderPath, '..'));
                });
        } else {
            // Wrap in a promise just in case first call to self function would be rejected
            // for any other case we may just return Error object
            return deferred(new Error("No such file: " + filename));
        }
    }(fDepth, folderPath));
};

:)

zemd commented 12 years ago

Your function is great! There is no need for special "reject" method now, but I will try find more cases :)

Also why do you choose deferred.promisify for path.exists? I think using deferred logic it should be call fail callback when exists === false. It is not problem, but this is interesting for me too :)

Thanks for your reply.

medikoo commented 12 years ago

deferred's success/error has exactly same semantics as success/error in Node function callbacks, in my opinion it should not be perceived as something different.

You may of course create fake errors and make some use of error flow, but I think it's not good and it rises other issues. If logic needs to be split. I would definitely do it in success callback:

promise.then(function (something) {
  if (something) {
    // go that way
  } else {
    // go other way
 }
}, function (err) {
  // Handle error (corner case)
});

What might be good, is to add some other extension to promise, which would allow to provide three callbacks ontrue, onfalse, onerror , it might be named if, then pathExists call from your function would look like:

return pathExists(filePath).if(filePath, function () {
  return self(--fDepth, PATH.join(folderPath, '..'));
});

Not sure though about if name, it won't work in old engines (reserved keyword) and logically it should rather be then ..but hey then is already used for separating success/error flow :)

zemd commented 12 years ago

thanks for explanation, this is one more step for me understanding deferred :)

"if" extension would be good solution for this kind of work. you can extend "then" function by passing expression param: promise.then(expr, ontrue, onfalse, onerror), so when you use expression - you can have one behavior, and when this is just 2 callbacks then standard behavior.

medikoo commented 12 years ago

With promise.then it wouldn't work, and cumulating many functionalities in one function would be confusing. I'll think about if extension, as it indeed it might be helpful.