-
Notifications
You must be signed in to change notification settings - Fork 363
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
Do not use AbortController in the worker if not available #679
Changes from all commits
6d40b02
e8b5085
d86e597
99de201
d121ec2
42c6391
facb235
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1 @@ | ||
export const isIE11 = () => /Trident.*rv:11\.0/.test(navigator.userAgent); | ||
|
||
export const isSafari10 = () => | ||
/AppleWebKit.*Version\/10/.test(navigator.userAgent); | ||
|
||
export const isSafari11 = () => | ||
/AppleWebKit.*Version\/11/.test(navigator.userAgent); | ||
|
||
export const isSafari12_0 = () => | ||
/AppleWebKit.*Version\/12\.0/.test(navigator.userAgent); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,15 +46,19 @@ const messageHandler = async ({ | |
}); | ||
} | ||
|
||
const abortController = new AbortController(); | ||
const { signal } = abortController; | ||
let abortController: AbortController; | ||
|
||
if (typeof AbortController === 'function') { | ||
abortController = new AbortController(); | ||
fetchOptions.signal = abortController.signal; | ||
} | ||
|
||
let response: any; | ||
|
||
try { | ||
response = await Promise.race([ | ||
wait(timeout), | ||
fetch(fetchUrl, { ...fetchOptions, signal }) | ||
fetch(fetchUrl, { ...fetchOptions }) | ||
]); | ||
} catch (error) { | ||
// fetch error, reject `sendMessage` using `error` key so that we retry. | ||
|
@@ -67,7 +71,12 @@ const messageHandler = async ({ | |
|
||
if (!response) { | ||
// If the request times out, abort it and let `fetchWithTimeout` raise the error. | ||
abortController.abort(); | ||
if (abortController) abortController.abort(); | ||
|
||
port.postMessage({ | ||
error: "Timeout when executing 'fetch'" | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we adding this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was mostly for the benefit of the test, to be able to determine that the failure was because of a timeout, but could also be a useful error message on the client regardless. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine. It rejects the promise using the same error we already have incase it timesout based on the setTimeout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't do.. if the timeout is hit, no response will be returned from |
||
|
||
return; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frederikprijck is it fair to say we only added these Safari checks to stop it from using the web worker purely because of
AbortConrtoller
, or were there other factors?I haven't removed these functions, do you see value in keeping them around or should we remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to fix the AbortController: #594
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we revert this, we should test this again on Safari. LMK if you dont have access to BrowserStack, I dont mind testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've tested, versions I tested are in the PR