I'm not sure about bluebird, but native promises for sure are not usable. We rely on synchronous state checking on the promises in the C++ side, which native promises don't implement. You basically have to be able to do something like one of these

mypromise.isPending() // true or false
myPromise.state // == pending/resolved/rejected

one way or the other (single value of state, or multiples for is pending or is resolved etc), and it can be a function or a prop, but it HAS to be synchronous

This is so we can use promise returning functions inside callbacks that get invoked from inside C++. We have to be able to check the status synchronously so we can sleep the thread that the main callback is handled in to allow more ticks to go through handling promise callbacks.

Does bluebird allow this functionality?

yea, it does...

have a look here https://github.com/petkaantonov/bluebird/blob/634af0e27ff4faab62c6c5bfd105527abcf8b06e/src/synchronous_inspection.js

https://github.com/petkaantonov/bluebird/blob/4576d0b18d5bff5194d74be5b4530e275c12912e/API.md#synchronous-inspection

you can get the state, value, whatever :)

also, appears this is a WIP cause tests are broken... locally, I have not run into any errors so far. take a look in a moment

EDIT: travis complains about the double quotes. I fixed that a bit ago and force-pushed. I'm gonna try force pushing again..

cheers

ok this is weird. locally, it says all tests pass, but I see this

Unhandled rejection AssertionError: 3 == 2
    at /Users/kenny/Projects/github.com/heavyk/nodegit/test/tests/diff.js:229:14
    at tryCatcher (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/util.js:24:31)
    at Promise._settlePromiseFromHandler (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/promise.js:454:31)
    at Promise._settlePromiseAt (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/promise.js:530:18)
    at Promise._settlePromises (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/promise.js:646:14)
    at Async._drainQueue (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/async.js:182:16)
    at Async._drainQueues (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/async.js:192:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/Users/kenny/Projects/github.com/heavyk/nodegit/node_modules/bluebird/js/main/async.js:15:14)
    at processImmediate [as _immediateCallback] (timers.js:371:17)

for the failed test passing, perhaps that was a change that was made on the runner. I'm looking now.

for the state needing to be read in C++, I obvously didn't implement that, so that's likely broken.
any ideas on where I can find that code?

I'll have to dig around, I forget the exact location.

that function is the one that (presuming we already have a .then function on our object, we've assumed we've got a promise) checks for the promise being resolved, and sets baton->done to true.

well, I'm testing right now and they seem to be working just fine..

> var p = require('bluebird').resolve('lala')
undefined
> p.isPending()
false
> p.isFulfilled()
true
> p.value()
'lala'
> p = Promise.resolve('lala')
Promise { 'lala' }
> p.isPending()
TypeError: p.isPending is not a function
[...]

I'm trying to figure out why diff is giving different results now... (it didn't before, so maybe I messed something up)

Hey @heavyk! It looks like the linter is failing for this. I'm not opposed to switching to bluebird but I think that currently we cannot use native Promise implementation due to our reliance on sync promise polling.

@johnhaley81 yes, I just realized I forgot to push the fixes for the double quotes. force pushed an update. should be all good now.

there's still an outstanding "thing" with the diff test it seems. I'll have a look tomorrow. it's rather late here

cheers

omg this is embarrassing... I totally didn't pay attention. here's why it's failing. I meant to find a solution to this before putting the PR... here's the patch anyway

thenables/thenify#6

I'll update the package.json to use my fix, (just temporarily so you guys can test it)... tomorrow I'll solve the problem. one sec

ok, code is ready for review now. there is one outstanding "thing" that I'm not sure about. it's this

Unhandled rejection AssertionError: 3 == 2
    at /home/travis/build/nodegit/nodegit/test/tests/diff.js:229:14
    at tryCatcher (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/util.js:24:31)
    at Promise._settlePromiseFromHandler (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/promise.js:454:31)
    at Promise._settlePromiseAt (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/promise.js:530:18)
    at Promise._settlePromises (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/promise.js:646:14)
    at Async._drainQueue (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/async.js:182:16)
    at Async._drainQueues (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/async.js:192:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/home/travis/build/nodegit/nodegit/node_modules/bluebird/js/main/async.js:15:14)
    at processImmediate [as _immediateCallback] (timers.js:371:17)

I also get it locally. but sometimes I also get this error.

./node_modules/mocha/bin/mocha test/runner test/tests/diff.js


  Diff
    ✓ can walk a DiffList
    ✓ can diff the workdir with index
    ✓ can resolve individual line chages from the patch hunks
    ✓ can diff the contents of a file to a string
    1) can diff with a null tree
    ✓ can diff the initial commit of a repository
    ✓ can diff tree to index
    ✓ can diff index to workdir
    ✓ can find similar files in a diff


  8 passing (6s)
  1 failing

  1) Diff can diff with a null tree:

      AssertionError: 84 == 85
      + expected - actual

      -84
      +85

      at test/tests/diff.js:186:16

sometimes.

you see, none of my code actually uses diffs (yet). also, when I try and print the diff out they're all empty. so, I really have no idea what's going on there. I neither understand why a null tree will produce 85 diffs.

anyway, I need to get back to my normal work right now. does anyone have any idea what could be wrong there?

cheers

The "diff with null tree" basically just lets you use the default which is your current tree diff'ed with the tree you pass in. I'm not sure why yours is failing though.

Also, the can find similar files in a diff test is wrong. I disabled that in my PR https://github.com/nodegit/nodegit/blob/async-diffs/test/tests/diff.js#L274 which hasn't been merged into master yet.

aha, I see that.
I have rebased against master. locally all is well.
(that failure appears after I commit something or do something in the libgit repo itself)
let's see what travis says

ok travis is happy. the only outstanding issue is that there's a problem thenify-ing the module as-is -- and that's because some of the functions on the api are named delete. since delete is a reserved word, the promisified function cannot have the same name.

as a result, I put the hack here: heavyk/thenify@b9d7956

the other alternative would be to make another macro/inline function that does exactly the same thing as NODE_SET_METHOD but modifies the function name if it's a reserved word... although that is "better" (because you don't want to have your functions have a reserved name in the first place) I opted-out of this option. it would require maintaining duplicated functionality to what is already provided in the node.h

ideas?

Why does that function need a name at all? If anything, I'd suggest just adding a suffix to it. Something like wrapper, so that the internal method is called: deleteWrapper. Just seems silly to take property names and try and make identifiers out of them. They aren't tied to the same semantics.

let's step through this one by one:

• there is a method in libgit2 called delete (method of branch see branch.cc:34)
• in C++ it calls NODE_SET_METHOD(object, "delete", Delete);
• now look at https://github.com/nodejs/io.js/blob/master/src/node.h#L245 -- see how it sets the name?
• so when I use thenify, see: https://github.com/thenables/thenify/blob/master/index.js#L50

it throws an error. try it yourself:
using upstream thenify:

> require('thenify-all')(require('./build/Release/nodegit').Branch)
SyntaxError: Unexpected token delete
    at thenify (/Users/kenny/Projects/github.com/heavyk/nodegit-normal/node_modules/thenify-all/node_modules/thenify/index.js:17:15)
    at /Users/kenny/Projects/github.com/heavyk/nodegit-normal/node_modules/thenify-all/index.js:55:65
    at Array.forEach (native)
    at promisifyAll (/Users/kenny/Projects/github.com/heavyk/nodegit-normal/node_modules/thenify-all/index.js:53:11)
    at thenifyAll (/Users/kenny/Projects/github.com/heavyk/nodegit-normal/node_modules/thenify-all/index.js:19:10)
    at repl:1:23
    at REPLServer.defaultEval (repl.js:154:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)

using my repo of thenify:

> require('thenify-all')(require('./build/Release/nodegit').Branch)
{ create: [Function: create],
  delete: [Function: $$delete],
  isHead: [Function: isHead],
  iteratorNew: [Function: iteratorNew],
  lookup: [Function: lookup],
  move: [Function: move],
  name: [Function: name],
  setUpstream: [Function: setUpstream],
  upstream: [Function: upstream] }

it's kind of a silly bug really, because there's literally no way to make that happen without involving C++

> var fn = function delete() {}
SyntaxError: Unexpected token delete
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:131:25)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:209:10)
    at REPLServer.Interface._line (readline.js:548:8)
    at REPLServer.Interface._ttyWrite (readline.js:825:14)
> var fn = function() {}
undefined
> fn.name = 'delete'
'delete'
> fn.name
''

OH, I figured out a way, I think... lemme try real quick :)

Huh, I'm still not sure why that function needs a name, sorry. We only deal with properties, which can be assigned anonymous functions just fine. This feels like a bug with thenify.

FWIW this is why I wrote promisify-node, which does not have this issue. Why are we trying to introduce a different module that breaks?

I certainly see the potential for one of the most popular and well supported (and still very fast) libraries handling it. It's one less thing for us to directly have to maintain.

yes, so it would work just the same with promisify-node - I should have actually kept that.
for whatever reason, I felt it was necessary to clean up the ensureDir hacks and the like.
additionally, it would give a performance improvement by evaluating the function once at compile time and only promisifying necessary functions.

I thought that was win-win...

on hindsight, I see now that the performance improvement is virtually none...
my fix works. I'll put it up separately so you can see what I've done.

cheers

P.S. sorry for complicating the matter, lol

@maxkorp my issue isn't with bluebird, it is with thenify. i have no issue swapping out for Bluebird, as it is arguably a better Promise implementation than native

@tbranyen is right. anyway, I put a small and pretty tight version into the libgit.js template so it'll only affect the C++ promisifying bits.

I hope that's a good compromise.

in general, I like thenify and use it everywhere that I need to use generators. we were only held back by an obscure bug that cannot happen in plain js. so, I think it's appropriate to do it this way.

EDIT: had another bug with default ... seeing if this is the last one..
EDIT2: stupid linter... the lines were shorter before it reformatted them.. fixing..

With 60 files changed, I think we should all make sure we've reviewed this sufficiently before merging. Is this ready in your mind for a full review @heavyk?

most of those changes are require("bluebird") anyway but yep, I am satisfied with it :)

yesterday, I actually switched out to my branch for my daily tasks. it's solid.

oh yeah, my next PR will probably target error messages.

just a quick review of my code, I noticed that in examples/merge-with-conflicts.js I require fs

that was a bit of testing that snuck in. shall I apply this patch and rebase?

diff --git a/examples/merge-with-conflicts.js b/examples/merge-with-conflicts.js
index 9ac9409..8eb46b2 100644
--- a/examples/merge-with-conflicts.js
+++ b/examples/merge-with-conflicts.js
@@ -2,7 +2,6 @@ var nodegit = require("../");
 var path = require("path");
 var promisify = require("thenify-all");
 var fse = promisify(require("fs-extra"), ["remove", "ensureDir", "writeFile"]);
-var fs = require("fs");

 var repoDir = "../../newRepo";
 var fileName = "newFile.txt";
@@ -163,7 +162,7 @@ fse.remove(path.resolve(__dirname, repoDir))

     // if the merge had comflicts, solve them
     // (in this case, we simply overwrite the file)
-    fs.writeFileSync(
+    fse.writeFileSync(
       path.join(repository.workdir(), fileName),
       finalFileContent
     );
diff --git a/package.json b/package.json
index db490f9..1fbee2d 100644
--- a/package.json
+++ b/package.json
@@ -57,7 +57,6 @@
     "bluebird": "^2.9.30",
     "fs-extra": "^0.18.2",
     "node-pre-gyp": "^0.6.5",
-    "npm": "^2.9.0",
     "thenify-all": "^1.6.0",
     "which-native-nodish": "^1.1.1"
   },
@@ -71,6 +70,7 @@
     "lodash": "^3.8.0",
     "mocha": "^2.2.4",
     "nan": "^1.8.4",
+    "npm": "^2.9.0",
     "nw-gyp": "^0.12.4",
     "pangyp": "^2.1.0",
     "request": "^2.55.0",

(the npm part was discovered yesterday that it is only used by the clean script, so therefore it can be a devDep)

Wow, perfect timing. Just stumbled onto this much needed library and bluebird upgrade! Question from someone just diving into this specific project and PR. Why use thenify-all ? Bluebird has a promisify feature that does the same thing (from what I can tell). That would remove one more dependency.

I was about to post this:

var Promise = require('bluebird')
var fs = Promise.promisifyAll(require('fs-extra'))

From node-fs-extra

If that works with promisify-ing the library then I'd say lets rip out then-ify.

@Spidy88 true dat. @johnhaley81 do you want me to change that? I certainly can.

I would like to do it in a separate PR though, because it requires function name changes (eg. fse.remove -> fse.removeAsync) this is because the functions cannot retain the same names in interest of being able to promisify object methods. Promise.promisifyAll(new SomeObj) will still have the same this

so originally, I opted to just switch out the require statements, and not have to change the names of all the functions, with the intention of following up at a later time with that PR. it would have made the PR another 200-300 lines more...

so, yeah just let me know...

Sounds good to me.

@heavyk Ya, definitely something for another PR. Though I will point out, if we'd like to do minimal work and updates, you can provide a custom suffix for promisified functions. I'm assuming this means an empty suffix as well which would not add Async to the method name.

You can also provide a filter so that only a limited number of methods get promisified. I think again this would be the preferred method.

Final note, you could always do it this way as well:

fse.remove = Promise.promisify(fse.remove);

@Spidy88 can you do that? I'm not sure actually.
I think I remember that didn't work for me a while back.
I'm 100% sure it won't work with an empty suffix though.
you can see why if you look at the generated source
(handy to have this in my repl history, hehe)

> console.log(require('bluebird').promisifyAll(require('./build/Release/nodegit').Branch).deleteAsync.toString())
function (_arg0,_arg1,_arg2) {
            'use strict';
            var len = arguments.length;
            var promise = new Promise(INTERNAL);
            promise._captureStackTrace();
            var nodeback = nodebackForPromise(promise);
            var ret;
            var callback = tryCatch(this != null ? this['delete'] : fn);
            switch(len) {
                case 0:ret = callback.call(this, nodeback); break;
case 1:ret = callback.call(this, _arg0, nodeback); break;
case 2:ret = callback.call(this, _arg0, _arg1, nodeback); break;
case 3:ret = callback.call(this, _arg0, _arg1, _arg2, nodeback); break;

        default:
            var args = new Array(len + 1);
            var i = 0;
            for (var i = 0; i < len; ++i) {
               args[i] = arguments[i];
            }
            args[i] = nodeback;
            ret = callback.apply(this, args);

            break;

            }
            if (ret === errorObj) {
                promise._rejectCallback(maybeWrapAsError(ret.e), true, true);
            }
            return promise;
        }

you can see there that it's calling this['delete'] -- so I'm going to venture a guess that your suggestion won't work either. I'll still see what I can do.

so, I'll do the followup PR on monday.
I'm gonna chill a bit tomorrow.

cheers

Overall I'm 👍 on this. I'd like to also remove the thenify library but doing that in another PR would make sense. That will be a big change but I'd rather move everything to bluebird if possible.

I think we're good to merge this in. Any objections @maxkorp @tbranyen ?

right on! I'll whip out the next PR asap. I've been doing mostly client code lately, so it'll be a nice change... side note: we've been using this branch on our servers for about a month now without a single hiccup. it's solid.

oh, and I thought of some (ingenious) ways that I may be able to write a promisify which maintains the function name and stays optimized. we'll see if it works :) I was inspired by this https://github.com/isaacs/node-graceful-fs/blob/master/fs.js

Awesome! I'll go ahead and merge this then.