xmppjs / hubot-xmpp

XMPP adapter for Hubot
181 stars 103 forks source link

Add id to iq ping stanza #93

Closed pixelrebel closed 8 years ago

pixelrebel commented 8 years ago

Increments a new id for each ping. Currently, no id is provided so the response from the server is: [Tue Oct 06 2015 12:18:47 GMT-0700 (PDT)] ERROR [xmpp error]<iq type="error" xmlns:stream="http://etherx.jabber.org/streams"><error type="modify"><bad-request xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/><text xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" xml:lang="en">Missing 'id'</text></error></iq>

anupdhml commented 8 years ago

I think it's safe to have a static id, something like hubot_ping. IDs when sent from hubot are supposed to be ignored under XMPP, as we discussed in https://github.com/markstory/hubot-xmpp/issues/63#issuecomment-146046947.

markstory commented 8 years ago

@anupdhml An increment avoids goofy servers grumping about duplicate ids. I am not sure this is a real concern though.

pixelrebel commented 8 years ago

I used an increment because I saw the id incrementing in this example. However, slack was just as happy with a static id. Is there a concern for integer overflow using this increment method?

Frankly, after reading the xmpp spec, I'm starting to think this PR may not be the best idea. It's best to strictly adhere to the specs, right? Feel free to close this if you share the same concern.

markstory commented 8 years ago

Is there a concern for integer overflow using this increment method?

No, Javascript uses 64bit floats for numbers. Which gives millions of years at one ping per second.

sonnyp commented 8 years ago

@pixelrebel what's wrong about this PR regarding the spec?

BTW the spec was has a newer revision http://xmpp.org/rfcs/rfc6120.html

pixelrebel commented 8 years ago

@sonnyp Nothing, I suppose. Looks like id is a required attribute for iq stanzas. Shall we move forward with this PR?

Oh and thanks for the version update. A nice christmas present!

markstory commented 8 years ago

There are some merge conflicts. Can you fix them @pixelrebel? If not I can do it before merging.

sonnyp commented 8 years ago

cool, thanks @pixelrebel

markstory commented 8 years ago

0.2.1 tagged and published to npm.

pixelrebel commented 8 years ago

Thanks gents!