[Core] Make AbortController/AbortSignal work in the browser with fetch #6045
Conversation
There was a problem hiding this comment.
With regards to your comment on the instanceof AbortSignal check, this would pass if you compared an abortSignal to the global AbortSignal in browsers, correct? And the check should work in node.js since there is no global AbortSignal?
I think I did see that I could get our custom AbortSignal to pass the instanceof check in browsers (unfortunately this still failed when passed to fetch) if I set the AbortSignal prototype to a native AbortSignal's prototype. To do this I had to instantiate an AbortController, then access the signal's prototype. This felt 'hacky' but if we wanted the instanceof check to work badly enough is one thing we could try.
| @@ -69,7 +69,13 @@ export class AbortController { | |||
| */ | |||
| constructor(...parentSignals: AbortSignalLike[]); | |||
| constructor(parentSignals?: any) { | |||
| this._signal = new AbortSignal(); | |||
| if (typeof window !== "undefined" && typeof window.AbortController === "function") { | |||
There was a problem hiding this comment.
Should we check self instead of window in case we're running inside a webworker?
There was a problem hiding this comment.
I think you're right... we need to abstract out these platform checks in a better way. We're checking window in a lot of places outside of this library.
Yes. However, if you imported AbortSignal from our library and checked instanceof that, it would report false.
Also yes.
My opinion is that what's in this PR is a bit weird, but should be manageable. We can document that AbortSignal in the browser will vend native AbortSignal instances for compatibility reasons, and so instanceof may not work as you expect. |
ramya-rao-a
requested review from
xirzec
and removed request for
ramya-rao-a and
ramya0820
|
Related to #5531 |
|
I'm on the fence. This feels messy, but maybe good enough to unblock browser scenarios? |
ghost
added
the
no-recent-activity
|
Hi @bterlson. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
ghost
closed this
|
Hi @bterlson. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass. |
This is accomplished by vending browser-native AbortSignals rather than custom AbortSignals. I'm not entirely sure this is a good approach. While what I have works, the vended signals aren't
instanceof AbortSignal. This could be surprising. E.g.AbortSignal.none instanceof AbortSignalis false. There may be other downsides I'm not thinking of yet.That said, I experimented with a number of techniques to subclass the built-in AbortSignal, and none of them worked particularly well. The only alternative to this design I can think of uses a browser map to completely swap our AbortController and Signal with their native versions in the browser, but then linked signals wouldn't be supported.