Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

add `send` callback feature + change README sample #63

Closed
wants to merge 19 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

afshinm commented Aug 13, 2012

1- Adding the send callback
3- Change and complete the README sample with callback function
4- Tests

Contributor

rauchg commented Aug 13, 2012

Needs tests. Remember to test for callbacks in buffered messages.

Contributor

afshinm commented Aug 15, 2012

Tests added.

Contributor

rauchg commented Aug 15, 2012

This doesn't consider transport upgrades. The packet could end up being drained by a different transport.

Contributor

rauchg commented Aug 15, 2012

Also, if transport drains more than once the callback will fire more than once.

Contributor

rauchg commented Aug 15, 2012

We actually don't need "one" test for this, it has to be tested very thoroughly.

@rauchg rauchg commented on an outdated diff Aug 20, 2012

lib/socket.js
@@ -207,6 +212,8 @@ Socket.prototype.clearTransport = function () {
Socket.prototype.onClose = function (reason, description) {
if ('closed' != this.readyState) {
+ this.packetsFn = {};
+ this.packetSeq = 0;
@rauchg

rauchg Aug 20, 2012

Contributor

Bad indentation

@rauchg rauchg commented on an outdated diff Aug 20, 2012

lib/transports/polling.js
@@ -96,7 +98,17 @@ Polling.prototype.onPollRequest = function (req, res) {
req.on('close', onClose);
this.writable = true;
- this.emit('drain');
+ //decide to send empty `drain` or not
+ if(req.query.sid && this.writable && this.seqIds.length > 0){
+ debug("acks received from client, emit drain")
+ var singleItem = this.seqIds.splice(0,1);
+
+ for (var i = 0, l = singleItem[0].length; i < l; ++i) {
+ this.emit('drain', singleItem[0][i]);
+ }
+ } else {
+ this.emit('drain');
@rauchg

rauchg Aug 20, 2012

Contributor

We use 2 space indents

Contributor

rauchg commented Aug 20, 2012

This is looking pretty sweet!

Contributor

afshinm commented Aug 20, 2012

Thanks. Indentation problems also fixed.

@rauchg rauchg commented on an outdated diff Aug 20, 2012

test/server.js
+
+ conn.send('b', function (transport) {
+ i++;
+ });
+ });
+ });
+
+ socket.on('open', function () {
+ socket.on('message', function (msg) {
+ i++;
+ });
+ });
+
+ setTimeout(function () {
+ expect(i).to.be(4);
+ done();
@rauchg

rauchg Aug 20, 2012

Contributor

indent

@rauchg rauchg commented on an outdated diff Aug 20, 2012

lib/transports/polling.js
@@ -96,7 +98,16 @@ Polling.prototype.onPollRequest = function (req, res) {
req.on('close', onClose);
this.writable = true;
- this.emit('drain');
+ //decide to send empty `drain` or not
+ if (req.query.sid && this.writable && this.seqIds.length > 0) {
+ debug("acks received from client, emit drain");
@rauchg

rauchg Aug 20, 2012

Contributor

single quotes

@rauchg rauchg and 1 other commented on an outdated diff Aug 20, 2012

lib/transports/polling.js
@@ -96,7 +98,16 @@ Polling.prototype.onPollRequest = function (req, res) {
req.on('close', onClose);
this.writable = true;
- this.emit('drain');
+ //decide to send empty `drain` or not
+ if (req.query.sid && this.writable && this.seqIds.length > 0) {
+ debug("acks received from client, emit drain");
+ var singleItem = this.seqIds.splice(0,1);
+ for (var i = 0, l = singleItem[0].length; i < l; ++i) {
+ this.emit('drain', singleItem[0][i]);
+ }
+ } else {
+ this.emit('drain');
@rauchg

rauchg Aug 20, 2012

Contributor

Is it necessary that the polling transport reports what sequence id it's flushed? If you think about it, the Socket should know this, as the polling buffer won't keep growing, the writeBuffer at the socket level will.

@afshinm

afshinm Aug 20, 2012

Contributor

Yes, Actually any transport has two type of drain now, with sequence id and without sequence id. When the drain emits, the listener get one parameter (sequence id) in callback and then we going to search that is there any callback function for this sequence id or not, if yes, we execute it.

@rauchg

rauchg Aug 20, 2012

Contributor

Sure but the transports don't really need to know about sequence ids.

@afshinm

afshinm Aug 21, 2012

Contributor

You mean always emit an empty drain event on polling transports? (Please clarify all transports or just polling)

@rauchg

rauchg Aug 21, 2012

Contributor

As far as Socket goes, it doesn't matter if it's polling or websocket, it just needs to wait on the drain event, for which it should know what the sequence id is.

Contributor

rauchg commented Aug 20, 2012

Tests still have bad indentation. Also, the new engine.io style is multiple var instead of var + commas. Thanks!

Contributor

afshinm commented Aug 21, 2012

Tests code styling also fixed with new engine.io rules. Lib files also reviewed again.

Contributor

afshinm commented Aug 24, 2012

Unnecessary functionality removed.

@rauchg rauchg commented on an outdated diff Aug 24, 2012

lib/socket.js
@@ -95,6 +95,7 @@ Socket.prototype.onPacket = function (packet) {
};
Socket.prototype.onError = function (err) {
+ this.packetsFn = [];
@rauchg

rauchg Aug 24, 2012

Contributor

I think it's redundant to have it both in onClose and onError

@rauchg rauchg commented on an outdated diff Aug 24, 2012

lib/socket.js
+ * Setup and manage send callback
+ *
+ * @api private
+ */
+
+Socket.prototype.setupSendCallback = function () {
+ var self = this;
+ //the message was sent successfully, execute the callback
+ this.transport.on('drain', function() {
+ if (self.packetsFn.length > 0) {
+ var seqFn = self.packetsFn.splice(0,1)[0];
+ if('function' == typeof seqFn) {
+ debug('executing send callback');
+ seqFn(self.transport);
+ }
+ }
@rauchg

rauchg Aug 24, 2012

Contributor

style problems

@rauchg rauchg commented on the diff Aug 24, 2012

lib/socket.js
if ('closing' != this.readyState) {
debug('sending packet "%s" (%s)', type, data);
this.writeBuffer.push({ type: type, data: data });
+ //add send callback to object
+ if('undefined' != typeof callback) {
@rauchg

rauchg Aug 24, 2012

Contributor

you can simply check that callback exists.

@rauchg rauchg commented on an outdated diff Aug 24, 2012

lib/transports/websocket.js
@@ -66,7 +66,7 @@ WebSocket.prototype.onData = function (data) {
* @api private
*/
-WebSocket.prototype.send = function (packets) {
+WebSocket.prototype.send = function (packets) {
@rauchg

rauchg Aug 24, 2012

Contributor

Unneeded whitespace

@rauchg rauchg commented on an outdated diff Aug 24, 2012

test/server.js
@@ -389,6 +389,8 @@ describe('server', function () {
});
});
+
+
@rauchg

rauchg Aug 24, 2012

Contributor

Unneeded \n's

@rauchg rauchg commented on an outdated diff Aug 24, 2012

@@ -1,4 +0,0 @@
-node_modules
-npm-debug.log
-coverage.html
-lib-cov/
@rauchg

rauchg Aug 24, 2012

Contributor

Why the changes to .gitignore?

@rauchg rauchg commented on an outdated diff Aug 24, 2012

@@ -17,7 +17,9 @@ var engine = require('engine.io')
, server = engine.listen(80)
server.on('connection', function (socket) {
- socket.send('utf 8 string');
+ socket.send('utf 8 string', function(transport){
+ //the message was sent successfully
@rauchg

rauchg Aug 24, 2012

Contributor

I don't think we want to show this so prominently. It's kindof an advanced-ish feature

@afshinm afshinm closed this Aug 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment