bluebird promises + thenify#615
Conversation
|
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 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 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 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. |
heavyk
changed the title
|
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.. 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 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 I also get it locally. but sometimes I also get this error. 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 |
also, add reserved word fixes to thenify (temporarily)
|
aha, I see that. |
heavyk
changed the title
|
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 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 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: |
|
let's step through this one by one:
it throws an error. try it yourself: using my repo of thenify: |
|
it's kind of a silly bug really, because there's literally no way to make that happen without involving C++ |
|
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 I thought that was win-win... on hindsight, I see now that the performance improvement is virtually none... 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 |
|
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 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 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 |
|
I was about to post this: 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. 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 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: |
|
@Spidy88 can you do that? I'm not sure actually. > 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 so, I'll do the followup PR on monday. 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. |
|
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. |
bluebird promises + thenify
Revert "Merge pull request #615 from heavyk/thenify-bluebird"
6 participants
this change does the following:
fse.ensureDirwere removed. further cleaning can be done.)some notes:
ref: petkaantonov/bluebird#667