-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add ability to abort a kfetch call. #20700
Changes from 2 commits
4e3f2dc
d293f87
d01c26f
94bf026
14228d8
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 |
---|---|---|
|
@@ -32,7 +32,16 @@ class FetchError extends Error { | |
} | ||
} | ||
|
||
export async function kfetch(fetchOptions, kibanaOptions) { | ||
export function kfetch(fetchOptions, kibanaOptions, isAbortable = false) { | ||
let signal; | ||
let abort; | ||
|
||
if (isAbortable) { | ||
const abortController = new AbortController(); | ||
signal = abortController.signal; | ||
abort = abortController.abort.bind(abortController); | ||
} | ||
|
||
// fetch specific options with defaults | ||
const { pathname, query, ...combinedFetchOptions } = merge( | ||
{ | ||
|
@@ -42,8 +51,9 @@ export async function kfetch(fetchOptions, kibanaOptions) { | |
'Content-Type': 'application/json', | ||
'kbn-version': metadata.version, | ||
}, | ||
signal, | ||
}, | ||
fetchOptions | ||
fetchOptions, | ||
); | ||
|
||
// kibana specific options with defaults | ||
|
@@ -57,17 +67,28 @@ export async function kfetch(fetchOptions, kibanaOptions) { | |
query, | ||
}); | ||
|
||
const res = await fetch(fullUrl, combinedFetchOptions); | ||
const fetching = new Promise(async (resolve, reject) => { | ||
const res = await fetch(fullUrl, combinedFetchOptions); | ||
|
||
if (!res.ok) { | ||
let body; | ||
try { | ||
body = await res.json(); | ||
} catch (err) { | ||
// ignore error, may not be able to get body for response that is not ok | ||
if (!res.ok) { | ||
let body; | ||
try { | ||
body = await res.json(); | ||
} catch (err) { | ||
// ignore error, may not be able to get body for response that is not ok | ||
} | ||
reject(new FetchError(res, body)); | ||
} | ||
throw new FetchError(res, body); | ||
|
||
resolve(res.json()); | ||
}); | ||
|
||
if (isAbortable) { | ||
return { | ||
fetching, | ||
abort, | ||
}; | ||
} | ||
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. What about attaching fetching.abort = abort; That way we can also get rid of 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 originally considered this approach but I had a few concerns about monkey-patching a native API:
These concerns are mostly based on principle so if you think it's more pragmatic to just monkey-patch the promise, let me know your thoughts because I can definitely be persuaded! OTOH if you think these concerns are worthy then I think creating a secondary API makes sense, because we're highlighting the fact that the consumer is consciously opting into secondary behavior. And realistically, do you think it will be used very often? Do we have many use cases for aborting a 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. These are very good points. I was mostly concerned about having two APIs but as you said, there won't be many use cases for aborting fetch, and in those cases it will be a conscious effort. I like the approach you suggested with a separate |
||
|
||
return res.json(); | ||
return fetching; | ||
} |
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.
Maybe abstract this into a separate
createAbortable
method that returns{ signal, abort }
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.
Good idea!