These are chat archives for deployd/contributors

28th
Jan 2015
Nicolas Ritouet
@NicolasRitouet
Jan 28 2015 11:19
@0ff awesome, thx for updating your PR
Sure thing, only there's a catch now: andreialecu's integration-tests won't work anymore. I don't see what's wrong, but I fear I have to pull the commit back for a while at least
Andrei Alecu
@andreialecu
Jan 28 2015 11:43
@0ff you need the .bind() call
that was the reason for using bluebird
actually not, I tried that :) it's a problem that somehow was not apparent when using ayepromise
in fact, I only call "events.emit('addCallback');" when a promise is actually waited for, i.e. when the user calles .then()
Andrei Alecu
@andreialecu
Jan 28 2015 11:45
my PR actually fixed a bug with that callback counting actually
as a side effect
yours doesn't have the same side effect and the bug will still exist unless fixed some other way
now that I always return a promise from the internal-client, but I don't call .then() for the "default async" case in get.js of internal-client-master, the default-async is not waited for anymore
Andrei Alecu
@andreialecu
Jan 28 2015 11:46
because i did addCallback by default on promises
yup, I saw that, and it's most probably the way to go for me
but I can imagine cases where I'd want to call a function that returns a promise and not want to wait for it implicitly
Andrei Alecu
@andreialecu
Jan 28 2015 11:48
how did the integration test fail exactly?
if you have a branch I could look at, I can help track it down
did you pull the commit?
pull it back I mean

imagine this as the test:

dpd.internalclientdetail.get({masterId: this.id}).then(function (data) {
    this.childrenPromise = data;
console.log('childrenPromise', this);
}.bind(this));


dpd.internalclientdetail.get({masterId: this.id}, function(data, err) {
    this.children = data;
console.log('children', this);
}.bind(this));

-> childrenPromise is logged, this contains childrenPromise
-> the request is ended and sent to the client
-> then children is logged, this contains children, but the request already ended

see, .get(data, cb) does not call the .then() and thus the call is not waited for
Andrei Alecu
@andreialecu
Jan 28 2015 11:50
oh I see what's going on
the problem is that you're calling finishCallback twice
when there's a promise
what I don't get is why this wouldn't happen with ayepromise
oh really, where?
oh
indeed
Andrei Alecu
@andreialecu
Jan 28 2015 11:52
it's possibly safe to just remove that else altogether
like 232-236
line* 232
or just push an empty function
or you'll have to check for empty fn before calling from line 88-90 in internal-client
I see, actually this will break balance between addCallback/finishCallback if left as is, or am I missing anything?
(i.e. there is a bug in master right now)
Andrei Alecu
@andreialecu
Jan 28 2015 11:53
that was the bug I referred to earlier
there is a bug in master right now when you call something like `dpd.email.post({ doc})
without a callback
it will decrement the callbackcount but not add a callback, so it will go out of sync
ah, yup, that's true - and removing it actually fixed integration tests
Andrei Alecu
@andreialecu
Jan 28 2015 11:54
my promise PR fixed this because I always incremented the callback counter anyway
I'll run all the tests again, but things look great
thanks a lot for your assistance!
Andrei Alecu
@andreialecu
Jan 28 2015 11:55
also for your commit, if you add bluebird, please remove Q
:)
my PR removed Q, you'd have to convert the other remaining places
I used bluebird-Q for it
hm, I would leave that for another PR (i.e. add bluebird here, remove it cleanly anywhere else in another PR)
Andrei Alecu
@andreialecu
Jan 28 2015 11:57
that might work
so that both PRs might be applied on their own
Andrei Alecu
@andreialecu
Jan 28 2015 11:57
alright
also one other thing to test
which I stumbled upon
with your implementation
try throwing an error inside the .then(function() { throw ... }) and see what happens
also throw an error from the regular callback/old style and see what happens
it should have the same behavior
it should, I'll do that right away
Andrei Alecu
@andreialecu
Jan 28 2015 12:04
and, please add that .bind() back in so that the integration test works without that extra bind at the end
otherwise there's no point of using bluebird anyway
might as well just have used Q
which is already in node_modules
but then we'd have to change all the calls to the internal baseMethods (as in your PR) to keep this, don't we?
I wasn't against using what was there :P
Andrei Alecu
@andreialecu
Jan 28 2015 12:05
yes, you can do that, copy paste from there
no problem as far as I'm concerned :)
for consistency's sake this is pretty important
you didn't actually need the .bind(this) for the second call in the integration test, that already works with current master
remove it and see
why is that? the old callback wasn't called in the same scope, was it?
Andrei Alecu
@andreialecu
Jan 28 2015 12:07
they both need to work the same
oh, okay, I'll try
Andrei Alecu
@andreialecu
Jan 28 2015 12:08
it's not something I added just for the sake of adding more magic, I just wanted it to be consistent
if I had a choice in designing this in the first place, I probably wouldn't have added all this magic, but we're stuck with it now :)
indeed, and for that reason you are totally right! there's the reason: https://github.com/deployd/deployd/blob/master/lib/script.js#L176
Andrei Alecu
@andreialecu
Jan 28 2015 12:09
yup
it's also in the documentation
So we might as well just bind the onFulfilled/onRejected correctly, though then we might as well go with what bluebird provides :)
in theory this should work as intended and provide the same context to the onFullfilled already
Andrei Alecu
@andreialecu
Jan 28 2015 12:11
interesting, is it?
(I see it's not working, just asking why :))
Andrei Alecu
@andreialecu
Jan 28 2015 12:11
the problem however is that you're only wrapping .then
there are other methods
like .finally()
and .done()
now that I think about it
there are now :P ayepromise was only return {then: ...} :D
so you are right, I'll return the correctly bound promise
Andrei Alecu
@andreialecu
Jan 28 2015 12:12
hm, I think you may need to wrap .finally too
monkey patch it
otherwise it won't wait on it
nop, I would change wrapPromise to return promiseable to return promiseable.bind(...)
Andrei Alecu
@andreialecu
Jan 28 2015 12:13
yes, that's a different issue
what I mean is
oh, i see
sorry (was thinking about this before realizing you changed your thought :D)
Andrei Alecu
@andreialecu
Jan 28 2015 12:14
dpd.internalclientdetail.get({masterId: this.id}).then(function (data) { this.childrenPromise = data; }).finally(function(){ this.finallyCalled = true; });
uh, formatting fail
anyway
will finallyCalled be in the response?
I'm not sure
not right now
the problem is that we'd be totally dependent on bluebird now (meaning we must do this for every method bluebird exposes) if I'm not mistaken
Andrei Alecu
@andreialecu
Jan 28 2015 12:16
that's why I think it was best
if you actually could just addCallback by default on the promise
and finishCallback on a finally
for the whole promise chain
instead of wrapping every single .then()
I'm not sure if that's possible
Andrei Alecu
@andreialecu
Jan 28 2015 12:20
it might be
so instead of monkey patching .then you can just call that using on the promise and finishcallback there
it would simplify the code further
I think I see another problem: what about functions that don't return bluebird promises, I mean what if they don't have a .finally()?
Andrei Alecu
@andreialecu
Jan 28 2015 12:21
you can convert them to a bluebird promise
do you know how? couldn't find it
Andrei Alecu
@andreialecu
Jan 28 2015 12:21
Q has Q(promise)
I think
let me check
I think this is it
ah nice!
Andrei Alecu
@andreialecu
Jan 28 2015 12:24
so you can pass it through Promise.resolve()
about using using: seems to only apply to resources as files, network connections, .... We might just use .finally as well, I guess
Andrei Alecu
@andreialecu
Jan 28 2015 12:25
I think using is alright, it takes a promise as a parameter
it seems clearer to use using
but it requires a disposer which we don't have
Andrei Alecu
@andreialecu
Jan 28 2015 12:26
I don't think so
it takes either a Promise or a Disposer
keyword or
Promise.using(Promise|Disposer promise, Promise|Disposer promise ..., Function handler) -> Promise
that's the signature
okay but then it's only syntactically different from just using finally, as it'll do just the same?
Andrei Alecu
@andreialecu
Jan 28 2015 12:28
probably the bluebird Promises already have the disposer code in them, which is called in a finally by default
the problem with finally is that if you have multiple finallys I don't think you can guarantee you'll add yours at the very end
hm
I see your point
Andrei Alecu
@andreialecu
Jan 28 2015 12:29
well, actually the obj passed to your isPromise() is probably a complete promise chain in the first place
so adding another .finally() to it might be safe
but the same would happen in using
Andrei Alecu
@andreialecu
Jan 28 2015 12:30
it just seems to me that the using is more clearly designed for this purpose
I'm a c# dev at heart :)
and I'd use using in c# to do something like this
we have no tool to know when our returned promise (no matter if created by using, or finally) is finished being handled (i.e. in finally a user might again call sth. async)
I'm with you, totally would do that, but I think it can't solve the problem here
neither method, actually
Andrei Alecu
@andreialecu
Jan 28 2015 12:31
if something async is called in a finally, if it's on the context, it would've already been wrapped
either as a promise or an old style callback
so it would addcallback/finishcallback
but the initial call would already be finished
wouldn't it?
what if we'd require the user to return a promise from their event if they want us to wait?
Andrei Alecu
@andreialecu
Jan 28 2015 12:33
if the callback counter is decremented after finally returns, whatever code is in finally will already have incremented the callback counter if it called something async
we could then use that promise and wait for it, instead of wrapping and patching poor monkeys :D
Andrei Alecu
@andreialecu
Jan 28 2015 12:34
so callback counter will be 1, then 2 from the async call in finally, then 1 again after the promise finished
then 0 when the async call in finally finished
but no
it'll be 1, then our finally calls = 0, then the user's finally runs?
Andrei Alecu
@andreialecu
Jan 28 2015 12:35
no because our finally will run last
but only if we'd patch it, because otherwise we'd return a promise which the user might bind on
Andrei Alecu
@andreialecu
Jan 28 2015 12:36
if you have this chain: dpd.internalclientdetail.get({masterId: this.id}).then().then().catch().finally();
okay
Andrei Alecu
@andreialecu
Jan 28 2015 12:36
what you get in your wrapPromise function is the entire chain, up to finally
so what you add to it, will be by default at the end
no it's not, because we get what .get returned
Andrei Alecu
@andreialecu
Jan 28 2015 12:37
no, those chained calls are synchronous
you can't intercept just the first one
but we only wrapped .get, didn't we?
yes, so we wrapped what the client's .get returned
so that the .then can bind on what we returned
Andrei Alecu
@andreialecu
Jan 28 2015 12:40
you're right
sorry
but
since that's where the deferred is actually created
maybe the using will work?
:D I fear it won't
but I'm all up for trying
just one more thing: what about requiring the user to return the promise?
I mean this would totally go in the direction "less magic"
but I for one think it's totally okay to write "return ..." and this would solve all the problems with promises in events
we'd only check once, no need to wrap anything
Andrei Alecu
@andreialecu
Jan 28 2015 12:43
I'm not sure I understand what to return
can you show an example?
return dpd.internalclientdetail.get({masterId: this.id}).then().then().catch().finally();
this way we'd always get the whole chain
Andrei Alecu
@andreialecu
Jan 28 2015 12:44
that would end the script there
could have an extra method instead
promise(dpd.internalclientdetail.get({masterId: this.id}).then().then().catch().finally())
yes, you'd need to be careful in using return, but you can always just use a deferred
Andrei Alecu
@andreialecu
Jan 28 2015 12:45
but I don't think it's needed
what isn't?
Andrei Alecu
@andreialecu
Jan 28 2015 12:49
seems like we may need this promise method, called like promise(dpd.internalclientdetail.get({masterId: this.id}).then().then().catch().finally())
on the context
then you have the whole chain
yes, this would probably work, questions is: is it feasible to reqiure the user doing this. I personally think it is, if it’s mentioned in the docs.
(sorry, I have to go right now, we should put this up for discussion I think)
Andrei Alecu
@andreialecu
Jan 28 2015 12:53
there is an alternative of adding support for this in bluebird with a PR
I could probably do it
“finalFinally”? :)
Andrei Alecu
@andreialecu
Jan 28 2015 12:54
somewhere you could hook into and intercept everything (.then/catch/finally/done/etc)
like your monkeypatching of .then but for everything
indeed, seems like somehting that might already be avaialble but under some obscure name :D
Andrei Alecu
@andreialecu
Jan 28 2015 13:02
it looks like there is a _then which is called for then/catch/done
Andrei Alecu
@andreialecu
Jan 28 2015 13:46
@0ff yeah it looks like this _then method is called for everything, so if you wrap that, everything in bluebird should just work: here's the signature: https://github.com/petkaantonov/bluebird/blob/master/src/promise.js#L200
I verified that even .finally() .tap() and everything else flows through this
I will feel an itching across all my skin when doing this, but it seems easy enough to do :)
Andrei Alecu
@andreialecu
Jan 28 2015 13:48
If you want, I can work on this too, push to your branch, I'll pull and poke at it some mor :)
more*
feel free :) I’ll have to go again, but I’ll take a look this evening
Andrei Alecu
@andreialecu
Jan 28 2015 13:50
just push your latest if you can
Eric Fong
@ericfong
Jan 28 2015 14:32
Just set the milestone of all PRs. Personally would like to launch the 0.8 first. Feel free to suggest which PR should go into 0.8. Thanks
Andrei Alecu
@andreialecu
Jan 28 2015 17:28
hey @0ff I sent you a PR
@andreialecu I just pushed the latest change, though this is only fixing some style issues. I did not push the removal of the else block, as it seems to me that this block is needed to actually finish the callback when not using promises. It seems the “last” finishCallback (i.e. count = -1) is used to trigger the done()
:D Talk about timing
Andrei Alecu
@andreialecu
Jan 28 2015 17:28
I think my PR is finished
all tests pass
I modified the integration test to confirm that finally also works
check it out
PR is here simpleTechs/deployd#1 if anyone else wants to look
Andrei Alecu
@andreialecu
Jan 28 2015 17:40
@0ff, the callback is finished from here: https://github.com/andreialecu/deployd/blob/feature/wrapPromises/lib/script.js#L232 when not using promises, the else block was most likely a bug, I think it's best removed
well I think (though I’m not 100% on this) that this is to have every function called supplied with a callback as the last argument. This will make sure that no matter what, every internal-client-function is waited for. Think dpd.collection.del({id:’123'}) - no callback would mean the script might be finished before everything’s done. With that else removed, there is no point at all where the done() fn is called, do you know what I mean?