Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial fix #237 in client and #474 in server for duplicated reconnection in JSONP #342

Closed
wants to merge 1 commit into from

Conversation

cris
Copy link

@cris cris commented Nov 23, 2011

Bug in client: #237
Bug in server: socketio/socket.io#474

Step to reproduce: socketio/socket.io#474 (comment)

This fix for handshake on client. Currently it fixes only JSONP kind of handshake. I'm using v0.8.4 and in this version for my case all handshakes goes through JSONP (comet.site.com <=> site.com).

But newer version of sio-client added for line: https://github.com/LearnBoost/socket.io-client/blob/master/lib/socket.js#L137
part: && !io.util.ua.hasCORS

Now, most of handshakes work through AJAX and maybe only Opera does handshake through JSONP.
And ajax-handshake isn't fixed :(.

There are two way to fix AJAX-part:

  1. Disable AJAX-handshake and use only JSONP-handshake
  2. Try to implement fix for AJAX-part

I dont know, what you think about it. Please give me feedback. If you think item 2 is preferred, I can try to implement clean solution for it.

@CameronJ
Copy link

CameronJ commented Dec 1, 2011

Very nice! Thanks! I'm running version 0.8.7, and to force everything to go through JSONP, I just changed the if block slightly that you mentioned from !io.util.ua.hasCORS to (true || (this.isXDomain() && !io.util.ua.hasCORS))

Since I'm running on the same domain, it would be nice to have the AJAX side of the if block fixed, but isn't the most horrible thing to do it this way. At least it works!!!

@PritamUpadhyay
Copy link

Hello,
I am using Version 1.0.
This bug is not fixed on this version.
May i have some help regarding this bug?

@jayconstanza
Copy link

thank you so much for that

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants