-
Notifications
You must be signed in to change notification settings - Fork 55
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
Connectivity check fixes & tests #184
Conversation
Looks great @SimonWoolf 👍 |
@@ -66,6 +68,8 @@ var ConnectionManager = (function() { | |||
params.echo = 'false'; | |||
if(this.format !== undefined) | |||
params.format = this.format; | |||
if(this.stream !== undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this special-case crap for one of the transport params was what I was trying to get away from with the previous commit. The idea is that the stream param is added to params along with all other given transport params. The only special-casing needed is to take notice of whether or not it's a stream when creating the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite clear what you want. It needs to be set as a connect params somewhere. If not in getConnectParams
, we could do it in CometTransport#connect
after the getConnectParams
call here if you prefer.
Or do you mean you want it added directly into the jsonp Request object, ie here? If so, not sure I agree - that will let us remove it from getConnectParams, but it means specifying it in two places (it still needs to be an attribute on the Transport as well as that's what the requestMode looks at), which isn't very DRY. Or am I misunderstanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be set as a connect params somewhere.
I was intending that it gets into params
only because it's in transportParams
and then, if it is still unspecified, the default for that transport is applied. But that disperses the logic into different places so in retrospect I'm fine with this.
4397093
to
b898da8
Compare
Uploaded a new internet-up.js file with |
b898da8
to
2b2e81c
Compare
(Turns out Cloudflare was caching it more or less permanently despite setting a 4 hour cache-control header, needed to log on to cloudflare and explicitly purge the cache for that individual file...) |
@SimonWoolf so if you're happy this can merge and be released. |
This partly reverts commit 7bc6aee. (revert the removal of the ability of the transport to force streaming off, as that broken jsonp. Keep the transportParams mixin.)
2b2e81c
to
3215e27
Compare
Closes #183 ; currently fails due to https://github.com/ably/infrastructure/issues/476