-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(cancel-query-request): added abort query request functionality after submitting a query #18387
Conversation
2a3b476
to
a585d98
Compare
}) | ||
const results = await Promise.all(pendingResults.map(r => r.promise)).catch( | ||
error => { |
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.
I think if we are catching and then throwing error, it's the same as not catching at all? maybe this can just be the await Promise.all? not sure.
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.
That's a great question, you could be right. Let me go ahead and give that another shot. I think I originally added it to help the error bubble out of the promise, but it could be that I had an issue in the catch at the end of all this
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.
Yup, you're totally right. I will ✂️ it out
|
||
private handleCancelClick = (): void => { | ||
this.props.onNotify(queryCancelRequest()) | ||
this.abortController.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.
just to be safe, it's probably best to check that abortController
exists, since this function might be called before handleClick
when abortController
is undefined
this.abortController.abort() | |
if (this.abortController) { | |
this.abortController.abort() | |
} |
ui/jest.config.js
Outdated
@@ -23,7 +23,7 @@ module.exports = { | |||
'ts-jest': { | |||
tsConfig: 'tsconfig.test.json', | |||
diagnostics: { | |||
ignoreCodes: [6133, 6192] // ignore unused variable errors | |||
ignoreCodes: [6133, 6192, 2339], // ignore unused variable errors |
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.
cut this part - we added it for testing
ui/src/notebooks/context/query.tsx
Outdated
setQueryToLoading: () => void | ||
} | ||
|
||
export const DEFAULT_SUBMIT_BTN_CONTEXT: SubmitQueryButtonContextType = { |
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.
does this need to be exported as a constant (doesn't look like it's used outside this file)? can this just be left here and named in camelCase
?
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.
Sure thing. I was going based on the previous convention but you're right in that it doesn't need to be exported
@@ -51,16 +52,25 @@ export const runQuery = ( | |||
dialect: {annotations: ['group', 'datatype', 'default']}, | |||
} | |||
|
|||
const controller = new AbortController() | |||
const controller = abortController || new AbortController() |
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.
i think you should cut this since it's duplicated in runQuery
and the result from there is passed in here. caveat emptor to anyone using this method - they'll need to pass in an abort controller - that's fine, since you're the only one using it right now.
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 eyes, I totally didn't see that. Will do
icon={icon} | ||
size={ComponentSize.Small} | ||
status={ComponentStatus.Default} | ||
onClick={() => this.handleCancelClick()} |
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.
onClick={() => this.handleCancelClick()} | |
onClick={this.handleCancelClick} |
df60034
to
eb1d918
Compare
b9152fd
to
649297a
Compare
649297a
to
419a9e5
Compare
419a9e5
to
649297a
Compare
ui/src/notebooks/context/query.tsx
Outdated
@@ -21,6 +21,10 @@ export interface QueryContextType { | |||
query: (text: string) => Promise<BothResults> | |||
} | |||
|
|||
export interface QueryContextType { |
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.
what is the difference between this and the one on line 20?
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.
Literally nothing. Good eyes, looks like I added something that shouldn't have been added
@@ -106,7 +106,14 @@ const isFromBucket = (node: Node) => { | |||
) | |||
} | |||
|
|||
export const executeQueries = () => async (dispatch, getState: GetState) => { | |||
export const setQueryToLoading = () => dispatch => { |
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.
I don't understand why this is a thunk and not just an action?
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.
Totally can be. Good call
if (e.name === 'CancellationError') { | ||
return results | ||
} catch (error) { | ||
if (error.name === 'CancellationError' || error.name === 'AbortError') { |
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.
👍
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.
This showed up cause of testing! There are situations where the request can be canceled before the other the catch
in runQuery
has a chance to catch it, so it would wind up here without properly being converted into a CancellationError
private handleCancelClick = (): void => { | ||
this.props.onNotify(queryCancelRequest()) | ||
if (this.abortController) { | ||
this.abortController.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.
I wonder what happens to abortController after you abort it? I'm wondering if you should be setting abortController to null here as well. not sure.
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.
That's a great question. Let me find out
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.
oooh great question! 🤔
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.
I could be wrong, but according to the docs it looks like once an abort controller is aborted, a rejected promise is returned? Although I'm not entirely certain what the means for the instantiated abortController:
https://dom.spec.whatwg.org/#abortcontroller-api-integration
I don't see why setting the value to null
would hurt
dispatch(saveDraftQueries()) | ||
dispatch(executeQueries()) |
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.
could you dispatch(setQueryToLoading) here instead of as part of the private handleClick = (): void => {
method in submit button?
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.
That sounds great!
649297a
to
a1132df
Compare
… made the onNotify optional
a1132df
to
597c50f
Compare
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.
this is much cleaner
…al bump in version caused alerts to break
@@ -1,4 +0,0 @@ | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
# yarn lockfile v1 |
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.
hrrm wait why is this being generated?
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.
@ebb-tide i removed since I had apparently introduced it by running yarn install at the root directory 7 months ago. Just came across it when I was working in fixing the issue that stemmed from the lock file and figured it should be removed to reduce confusion
Closes:
#18296
Problem
This PR aims to address two issues:
Solution