Add shouldSendCallback, set*Callback methods #220
Conversation
LewisJEllis
force-pushed
the
should-send-data
branch
from
7c1ee43 to
4bb2f3d
Compare
|
|
||
| // neither of these should fire, so report err to done if they do | ||
| client.on('logged', done); | ||
| client.on('error', done); |
There was a problem hiding this comment.
Won't this just end the test if they are invoked?
There was a problem hiding this comment.
done takes an err argument so if anything is passed to it the test fails; lets you easily say "as long as this callback isn't passed an error, we're good".
| var original = this[propertyName]; | ||
| if (typeof callback === 'function') { | ||
| this[propertyName] = function (data) { | ||
| return callback(data, original); |
There was a problem hiding this comment.
Can you add a test that verifies that original is passed if there is a pre-existing callback?
| this.send(kwargs, cb); | ||
| } else { | ||
| setImmediate(function () { | ||
| // wish there was a good way to communicate to cb why we didn't send; worth considering cb api change? | ||
| // avoiding setImmediate here because node 0.8 |
There was a problem hiding this comment.
Good point. Any thoughts on what this might look like?
There was a problem hiding this comment.
Could just make the eventId param be something like { eventId: ..., sent: bool, reason: string } but that seems kind of hacky. I think it's probably fine, I can't come up with a compelling reason that isn't satisfied by some extra checks/logic in the shouldSendCallback.
LewisJEllis
force-pushed
the
should-send-data
branch
from
4bb2f3d to
ab8ec24
Compare
| zlib.inflate(new Buffer(body, 'base64'), function (err, dec) { | ||
| if (err) return done(err); | ||
| var msg = JSON.parse(dec.toString()); | ||
| msg.extra.foo.should.equal('bar'); |
There was a problem hiding this comment.
Will this assertion / zlib.inflate run before done is called on L473?
There was a problem hiding this comment.
Caught me slippin again. I'm actually not sure, could go either way depending on implementation details. Switched to inflateSync to be more obviously sequenced.
LewisJEllis
force-pushed
the
should-send-data
branch
from
5d6c6e4 to
21f5293
Compare
|
wfm (although tests are failing) |
LewisJEllis
force-pushed
the
should-send-data
branch
from
21f5293 to
891bf7d
Compare
LewisJEllis
force-pushed
the
should-send-data
branch
from
891bf7d to
2c3be3e
Compare
No description provided.