Possibility to disable JSONP. #314
Conversation
…available transport.
|
Thoughts on |
|
Great work btw, especially on test support. |
|
what about one jsonp option to handle |
|
That would have been good had we not already had |
|
oh yeah backwards compatibility and whatnot. |
|
LGTM. I would have favored having two transports (since they really are two different ways of transporting the data backed by a common mechanism of xhr) but this is certainly the smaller code change. |
|
I agree with you @defunctzombie. Having just |
|
@guille so we should for now have an option like this one, and later refactor the transports so you don't necessarily ned polymorphism? Or should we go for the separation of polling already now?
|
|
Right. Moving forward we'd have |
|
Why would you add this? It doesn't solve anything, it doesn't provide any additional stability and allowsing people to connect with websockets first instead JSONP makes no sense for project which advertises that it's using known working transports before trying to upgrade to websockets.. |
|
I'm discussing the ability to disable JSONP in a cleaner way. |
|
@guille But why would you want to disable JSONP in the first place? |
|
I don't, but many people have asked for it. |
|
Renamed option and changed the constructor check to try-catch and error throw. |
|
Some people think it is insecure.
|
| var transport = this.createTransport(transport); | ||
|
|
||
| // Retry with the next transport if the transport is disabled (jsonp: false) | ||
| var transort; |
| @@ -71,6 +71,7 @@ function Socket(uri, opts){ | |||
| this.upgrade = false !== opts.upgrade; | |||
| this.path = (opts.path || '/engine.io').replace(/\/$/, '') + '/'; | |||
| this.forceJSONP = !!opts.forceJSONP; | |||
| this.jsonp = null == opts.jsonp ? true : !!opts.jsonp; | |||
|
@rase- pls update the readme |
|
Ah, will do. Thanks @mokesmokes. |
A first pass at being able to disable JSONP polling. Totally up for discussion.
Enables disabling by an option
noJSONPthat can be set totrue(falseby default). If set to true and trying to open a polling connection, we move immediately to a possible next transport. If there is not transport left to open with, we emit an error that no transports are available.We could make our polling transports also not autodetectable as
xhrandjsonso that we could specify transport literals like:['xhr', 'websocket']to specify we don't want JSONP polling as an option. This would, however, complicate the handling of transports and upgrades and I'm not sure if it's worth it for this one edge case.Thoughts on this @guille?