mkuklis / phonegap-websocket

Websocket PhoneGap plugin for Android
203 stars 78 forks source link

Native support detection fails in a desktop browser #32

Closed sebastien-p closed 10 years ago

sebastien-p commented 10 years ago

Hi, thank you for this plugin, it's awesome and totally saved my day!

I wrote a LiveReload plugin which uses phonegap-websocket so that it also works in older Android devices. But I also need it to work in desktop browsers so that developers can use $ cordova serve and access their app in a DevTools equipped browser.

The thing is, the way your plugin do WebSocket support detection, it replaces desktop browsers native WebSocket API with its own and, of course it won't work anymore because of Cordova not initializing its native code part.

So, what do you think of using another detection method like this one (binary version)?

mkuklis commented 10 years ago

@sebastien-p I'm glad the plugin helped. I'm open for other detection mechanisms but we will need to test it carefully. We ran into very ugly issues with this in the past. For example some older androids (before 4.4) have Websocket constructor present together with additional properties but the implementation of the methods are broken. I will have to take a closer look at this approach.

mkuklis commented 10 years ago

btw LiveReload looks cool!

sebastien-p commented 10 years ago

Thanks!

What about something like:

function hasWebSocket() {
  var m = /Android ([0-9]+)\.([0-9]+)/i.exec(navigator.userAgent);
  var hasConstructor = typeof WebSocket === "function";

  // But is it possible that some old Android WebViews don't expose the
  // `/Android ([0-9]+)\.([0-9]+)/i` pattern in their user agent string?
  // UA sniffing are hacks that should be avoided if possible.
  if (!m) { return hasConstructor; }

  var x = parseInt(m[1], 10);
  var y = parseInt(m[2], 10);

  return hasConstructor && (x > 4 || (x === 4 && y >= 4));
}
mkuklis commented 10 years ago

@sebastien-p this won't work unfortunately because like I mentioned before some older versions of android have broken WebSocket implementations. Please check https://github.com/mkuklis/phonegap-websocket/issues/28 (the bottom part gets interesting). Some versions provide WebSocket constructor without full implementation so in this case the first m condition will fail and then the hasConstructor will be true. UA is a hack but the one which works.

Maybe one approach here could be to just test first if the Android is part of the userAgent if not then fall back to constructor test.


function hasWebSocket() {
  var isAndroid = /Android/i.exec(navigator.userAgent);
  var m = /Android ([0-9]+)\.([0-9]+)/i.exec(navigator.userAgent);
  var hasConstructor = typeof WebSocket === "function";

  if (isAndroid) {
    if (!m) return false;

    var x = parseInt(m[1], 10);
    var y = parseInt(m[2], 10);
    return hasConstructor && (x > 4 || (x == 4 && y >= 4));
  }  else {
    return hasConstructor;
  }

I would be fine with this approach. Thanks.

sebastien-p commented 10 years ago

OK but I don't understand, why would you test the UA string for "Android" twice?

mkuklis commented 10 years ago

@sebastien-p sorry I think your example will work just fine. I misinterpreted it (I need to get some coffee). Please go ahead create a pull request. I'm happy to merge it.

sebastien-p commented 10 years ago

My example should work fine if the Android WebView always exposed "Android x.y" in its UA string. As I'm not that familiar with older Android devices, do you think it's the case?

mkuklis commented 10 years ago

Yes I think it should be fine for most cases :) http://www.useragentstring.com/pages/Android%20Webkit%20Browser/

sebastien-p commented 10 years ago

Thanks for the link! Here is the pull request: https://github.com/mkuklis/phonegap-websocket/pull/35.