Exclude Safari when validating if AbortController exists

homer0 commented 5 years ago

What does this PR do?

As reported on #23, Safari claims to support AbortController, but while the object is indeed present, calling .abort() doesn't do anything.

@mo tested it on the technical preview and the issue is still there, so the easiest solution we came up with is, on Safari, apply the polyfill even if AbortController exists.

How should it be tested?

I created this sandbox with a simple script that creates a request and aborts it right away.

You can test how it works on other browsers but doesn't on Safari.

In order to test the fix, you can:

Build this branch npm run build.
Download the sandbox (last icon on the top left)
Copy the dist to the sandbox's node_modules/abortcontroller-polyfill (or use npm/yarn link).
Run npm start on the sandbox.

You can start something from scratch too, but I think this is easiest.

Notes

I changed the RegExp a little bit because Chrome's user agent also ends with Safari/....
Before doing any change, I tried running the tests to make sure my changes wouldn't break anything but I got Failed: unknown error: call function result missing 'value', and since there wasn't a Selenium script for Safari (and I not any good with Selenium), I didn't test the change.

mo commented 5 years ago

I've been testing this patch today and I noticed that it doesn't work on iPad devices because those have a Mobile/XYZ thing between the version/ and safari/ parts. Also I tried "Chrome on iOS" and "Firefox on iOS" and noticed that they suffer from the same bug (since they use the same webkit rendering engine).

So could you do something like this instead:

export function nativeAbortControllerIsBroken(self) {
  return self.navigator && (
    (self.navigator.vendor && self.navigator.vendor.indexOf('Apple') !== -1) ||
    (
      self.navigator.userAgent && (self.navigator.userAgent.indexOf('CriOS') !== -1 || self.navigator.userAgent.indexOf('FxiOS') !== -1)
    )
  );
}

i.e. put that in some file, and then import and use that from the two places where you modified the condition earlier.

homer0 commented 5 years ago

@mo Hey! Sorry for the delay, I've been kind of busy the last couple of days.

Now, you are right, I forgot about iOS. I checked the apps you mentioned on my phone (plus the Google app.. which is not the same a Chrome :P) and I made this small "list":

[
  {
    "app": "Desktop - Safari",
    "ua": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0.1 Safari/605.1.15",
    "vendor": "Apple Computer, Inc."
  },
  {
    "app": "iOS - Safari",
    "ua": "Mozilla/5.0 (iPhone; CPU iPhone OS 12_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1",
    "vendor": "Apple Computer, Inc."
  },
  {
    "app": "iOS - Chrome",
    "ua": "Mozilla/5.0 (iPhone; CPU iPhone OS 12_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/70.0.3538.75 Mobile/15E148 Safari/605.1",
    "vendor": "Apple Computer, Inc."
  },
  {
    "app": "iOS - Google",
    "ua": "Mozilla/5.0 (iPhone; CPU iPhone OS 12_1 like Mac OS X) AppleWebKit/604.1.34 (KHTML, like Gecko) GSA/63.0.221691834 Mobile/16B92 Safari/604.1",
    "vendor": "Apple Computer, Inc."
  },
  {
    "app": "iOS - Firefox",
    "ua": "Mozilla/5.0 (iPhone; CPU iPhone OS 12_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) FxiOS/14.0b12646 Mobile/16B92 Safari/605.1.15",
    "vendor": "Apple Computer, Inc."
  }
]

I never thought on checking the vendor, and since all of them share it, I think checking just that would be enough, but I agree that an extra validation is also good, so I kept the one you proposed and added the "UA name" of the Google app.

Once change I made is that I used startsWith and a regular expression instead of indexOf; if you prefer indexOf, no worries, I can use that.

Let me know what you think!

mo commented 5 years ago

That looks great, merged to master now (I just expanded the commit message a bit to include some context). I also published v1.2.0 to npm.

Thanks for reporting and fixing this bug!

mo commented 5 years ago

Here is the relevant bugzilla entry for webkit: https://bugs.webkit.org/show_bug.cgi?id=174980

mo / abortcontroller-polyfill