Added promise support #298
Conversation
| "ref": "1", | ||
| "ref-struct": "1", | ||
| "nan": "2" | ||
| "ref-struct": "1" |
There was a problem hiding this comment.
NPM did reordered nan and ref-struct by itself. I can change it back if you'd like, but NPM seems to want it this way.
|
Hmm, the build is failing on node 0.10 because it doesn't have a builtin promise implementation. I could supply Edit: I'm going to register bluebird as the provider for the tests, with a note about node 0.10. Edit 2: that seems to be working. |
|
Am I correct that the only docs are in the wiki, which I can't package with a PR? In that case, I'll edit the wiki to add promise information after this merges. |
|
The appveyor builds appear to be failing because of another any-promise issue. I'm not quite sure how they work, but I'll try to figure it out. Edit: It appears to be due to the windows file listing order, which mocha uses. |
|
The build seems to be passing now. |
|
The appveyor test seems to be failing only on one node version (3.2), and on variadic args (which I didn't touch). I think it's a race condition; I'm going to retry the build. |
|
And it's now passing. Looks like there's a race condition in variadic args, or at least the tests for it Anybody have any comments on this PR, now that the builds are passing? |
|
Any thoughts @TooTallNate? This PR only adds a useful feature, it doesn't change or remove features, so in my opinion if you have nothing against this PR you should merge it. |
| @@ -6,6 +6,7 @@ | |||
| var assert = require('assert') | |||
| , debug = require('debug')('ffi:_ForeignFunction') | |||
| , ref = require('ref') | |||
| , Promise = require('any-promise') | |||
There was a problem hiding this comment.
Do we need any-promise? Node 0.10 is extremely old, and standard Promises were introduced in Node 0.12. Could we just deprecate 0.10? Node 6 just came out.
There was a problem hiding this comment.
We still test 0.10, and also a lot of users use bluebird which has .finally. Our promises should match the user's, that's the point of any-promise.
|
Didn't realize we're still testing Node 0.10. I'll make an issue about deprecating it. Thanks @PlasmaPower! |
|
Any updates? 😄 |
|
Node 0.10 still hasn't been deprecated, but at this point if we were inclined to merge this I'd be in favor of deprecating Node 0.10 or skipping the test on node 0.10, then not using |
With JS as a whole moving toward promises, I think it's important to natively support promises. The simplification of the promises tests vs the async tests shows this.
JSHint seems to be throwing errors all over the place, even for things I didn't touch. If I styled something wrong, please let me know.