Disable JSONP transport if document is undefined. #283
Conversation
This makes the library compatible with React Native. Fixes #280.
| JSONPTransport.isAvailable = function() { return true; }; | ||
| ConnectionManager.httpTransports['jsonp'] = ConnectionManager.transports['jsonp'] = JSONPTransport; | ||
| JSONPTransport.isAvailable = function() { return isSupported; }; | ||
| if(isSupported) { |
There was a problem hiding this comment.
If we do not make this change (ie we still add the jsonp member) then if anyone attempts to request jsonp they will get an error saying it isn't supported (which I think is preferable), instead of saying it's unrecognised.
There was a problem hiding this comment.
I disagree, I think what's been done is correct. All other transports only add themselves to the transports array if they're supported (and connectionManager occasionally assumes that). If someone using React uses transports: ['jsonp'] (though I don't imagine anyone would), they'll get a "no requested transports available" error, same as if someone using IE8 specified transports: ['web_socket'].
There was a problem hiding this comment.
I disagree, I think what's been done is correct.
Fair enough. Maybe I should go back and read my own code :(
|
One comment but otherwise LGTM. |
|
LGTM (though @tcard as a rule we generally don't check in generated files until we release a new version, eg so merge conflicts etc only have to be solved in one place rather than many). I'll merge this in once the transport fallback changes have been merged in, as I'll need to rebase this on top of that (transport array names have changed). |
|
Merged as 79f93f0 |
|
👍 |
|
@SimonWoolf mind adding something to Readme about the library supporting React Native & also Cordova? |
|
Thanks @SimonWoolf |
This makes the library compatible with React Native.
Fixes #280.