-
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
[Expressions] Skip rendering if the request was cancelled #49669
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
💚 Build Succeeded |
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 @ppisljar or someone more familiar with this should chime in, so I'm just leaving comments.
try { | ||
return await this.promise; | ||
} catch (e) { | ||
return { | ||
type: 'error', | ||
error: { | ||
type: e.type, | ||
name: e.type, |
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 I prefer type
to name
, since it's really the type of error that is being extracted here.
@@ -135,6 +135,9 @@ export class ExpressionLoader { | |||
this.setParams(params); | |||
this.dataHandler = new ExpressionDataHandler(expression, params); | |||
const data = await this.dataHandler.getData(); | |||
if (data.type === 'error' && data.error.name === 'AbortError') { | |||
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.
Someone more familiar with expressions should chime in here. I'm wondering what we should do in the case of an abort? Should we go back to the previous state? Not sure whether a plain return is the right thing here.
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 think we should do this and should not make abort error a special case in how the expression service behaves.
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.
also i think there are two scenarious when AbortError can happen: (or maybe even more)
- abort gets called from the outside (dashboard or something). in this case we should show the error, as its popssible visualization didn't even finish its first render, so there might be nothing else to show. Also we don't know if (and when) we'll get a chance to do the request again.
- we have request in progress but then we get another update call before the first request completed. we (internally) abort the first one and start a new one. This is where we don't want to show the error.
Closes #49661
Related to #49380