Skip to content
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

The present-but-broken AbortController was fixed in Safari Technology Preview Release 74 #26

Closed
macku opened this issue Jan 24, 2019 · 14 comments

Comments

@macku
Copy link

macku commented Jan 24, 2019

The upcoming new version of Safari will ship the native Abordable Fetch controller. The latest preview release 74 released recently has fixed the missing API:
https://developer.apple.com/safari/technology-preview/release-notes/

The reported issue #23 and #24 pull requests weter supposed to fix the broken API, but in the recent version of Safari Preview, the polyfill brakes the browser runtime.

image

@mo
Copy link
Owner

mo commented Jan 26, 2019

Ah right, if AbortController is fully working in the stable version; the polyfill needs to detect the working AbortController implementation and not install itself over it (while still installing itself on top of the broken AbortController versions in earlier Safari versions). PR welcome if you're up for it and otherwise I will take a look at this later at some point.

macku added a commit to macku/abortcontroller-polyfill that referenced this issue Jan 30, 2019
…ri and Technology Preview.

Starting from version 12.2 the Safari will ship the implemented version of the abordable fetch API
macku added a commit to macku/abortcontroller-polyfill that referenced this issue Jan 30, 2019
…ri and Technology Preview.

Starting from version 12.2 the Safari will ship the implemented version of the abordable fetch API
macku added a commit to macku/abortcontroller-polyfill that referenced this issue Jan 30, 2019
…ri and Technology Preview.

Starting from version 12.2 the Safari will ship the implemented version of the abordable fetch API
macku added a commit to macku/abortcontroller-polyfill that referenced this issue Jan 30, 2019
…ri and Technology Preview.

Starting from version 12.2 the Safari will ship the implemented version of the abordable fetch API
@macku
Copy link
Author

macku commented Jan 30, 2019

I have created a fix that is checking the version of the Safari browser on desktop and also checks if we use iOS mobile device.

@getify
Copy link

getify commented Jan 30, 2019

Could we justify avoiding this extra complexity by just assuming that since Safari has now fixed their regression of a broken implementation, that eventually (soon) all users will be on a corrected Safari and thus we don't need to patch Safari at all?

@macku
Copy link
Author

macku commented Jan 30, 2019

I guess we could if we are fine with shipping broken polyfill for the Safari 11 12. I was trying to use a more smart way of detecting the broken signal but it's an async operation and I don't think we can use it:

const isAbortBrokenPromise = new Promise((resolve, reject) => {
  const controller = new AbortController();

  controller.abort();

  fetch('/', {
    signal: controller.signal,
  }).then(resolve, reject);
});

isAbortBrokenPromise.then(
  () => console.log('broken'),
  () => console.log('not broken')
);

@getify
Copy link

getify commented Jan 30, 2019

If I understand you correctly, I think what I'm suggesting is different. It's going back to how this polyfill worked before the reported breakage in 12.0.2. In other words, this polyfill would still patch Safari 11 (since it has no AbortController at all, right?).

And in 12.0.2-12.0.3, where the broken AbortController is present but broken, this lib would not do anything... meaning breakage would exist in that version window. But starting with 12.0.4 (or whatever), the real AbortController starts working.

So it's really only breakage in that version window of 12.0.2-12.0.3 or whatever. And I was asking if that breakage could be tolerable, given that most users will fairly quickly be upgraded to a Safari version that has a working AbortController?

@macku
Copy link
Author

macku commented Jan 30, 2019

(since it has no AbortController at all, right?)

Sorry, my bad. The Safari 11 indeed doesn't ship the native AbortContoller. I've typed the wrong version in my previous comment.
There is an AbortController in Safari 12 but it doesn't work with fetch. I'm not sure why it was shipped in the first place but the integration with fetch API is missing.
https://caniuse.com/#feat=abortcontroller

I was asking if that breakage could be tolerable

I will leave answering that question to the author of the polyfill. In my opinion, this might be problematic for some time only.

@mo mo changed the title The Abordable fetch controller was fixed in Safari Technology Preview Release 74 The present-but-broken AbortController was fixed in Safari Technology Preview Release 74 Jan 31, 2019
@chen86860
Copy link

Same issues in iOS 12.2 beta :(

Hope guys can fix it.

mo added a commit that referenced this issue Feb 3, 2019
… Safari 12.0.x, 12.1.x specifically but not >= 12.2

The native AbortController in Safari 12.2 and above works (starting with Safari Technology Preview Release 74) so the
polyfill should not install for these versions.

The latest versions of Chrome/Firefox for IOS still have the "present but broken" native AbortController so always
polyfill over their native implementations for now (the symptom otherwise is that cancelling never works, the HTTP
call happens even if the .abort() happens before the fetch() and the fetch promise is not rejected.

This commit fixes github issue #26
@mo
Copy link
Owner

mo commented Feb 3, 2019

Fixed on master now and released to npm as v1.2.3

The fixed version should "just work" on all browsers / browser versions, including Safari 12.0.x, 12.2 etc and Chrome/Firefox for IOS.

Please file another github ticket if you find any remaining issue for some browser / browser version.

@mo mo closed this as completed Feb 3, 2019
@kgusarov
Copy link

kgusarov commented Feb 6, 2019

Problem still here... Also, this solution looks kinda... broken to me:

  if (userAgent.match(/ Safari\//i) && (
    userAgent.match(/ Version\/12.0/i) ||
    userAgent.match(/ Version\/12.1/i)
  )) {
    return true;
  }

I mean, what will happen, when new Safari version will appear - new release? Maybe, it is better to think of other solution here...

51136439_550111022160272_4688328713392095232_n

@mo
Copy link
Owner

mo commented Feb 6, 2019

@kgusarov this fix handles the broken AbortController in 12.1.x and also in all 12.2.x releases (even future point releases and major releases). When a stable 12.3 is released it will have the working native AbortController.

So the fix should work for your 12.1 release too. Can you enter "navigator.userAgent" into the devtools console and paste the output here?

@kgusarov
Copy link

kgusarov commented Feb 6, 2019

@mo
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1 Safari/605.1.15 - should in theory fall under regex...

@mo
Copy link
Owner

mo commented Feb 6, 2019

@kgusarov do you have any "user agent switching" browser extension installed or some "privacy / anti tracking" browser extension that prevents JS from reading navigator.userAgent? I assume you have already double checked that you really have version 1.2.3 and that there isn't some cache issue that prevents the latest version of the polyfill from loading.

@kgusarov
Copy link

kgusarov commented Feb 7, 2019

@mo this problem is on our client's laptop, he was nice enough to bring to our office(!) for us to check the problem. No extensions are used there, and even dev tools were initially disabled. We checked everything, and the fix still doesn't work.

@youennf
Copy link

youennf commented Feb 8, 2019

Thanks for fixing the polyfill.
As of the check, one could also feature detect the presence of 'signal' in the Request prototype.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants