Skip to content
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

Panel: abort overlapping Fiber requests #6674

Draft
wants to merge 1 commit into
base: develop-minor
Choose a base branch
from

Conversation

distantnative
Copy link
Member

Description

Summary of changes

  • Abort previous Panel Fiber request if it is still running when new gets started

Reasoning

@distantnative distantnative added the needs: discussion 🗣 Requires further discussion to proceed label Sep 14, 2024
@distantnative distantnative self-assigned this Sep 14, 2024
@@ -67,7 +67,7 @@ export const states = [
export default {
create(plugins = {}) {
// props
this.isLoading = false;
this.controller = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion I would probably call it abortController to make instantly clear what this is controlling.

@bastianallgeier
Copy link
Member

Looking at the code, I was wondering if we could put that abort logic into the request.js module in a good way. Maybe even with an option to make requests abortable by demand via an option? That option could be set to false by default to keep the current concurrent behaviour, but for the open request we could then set it to true.

@distantnative
Copy link
Member Author

@bastianallgeier I think the deeper we move it, the trickier it could be to differentiate between which requests should cancel each other out. I think a pure boolean wouldn't work, e.g. search requests should abort each other and view requests should abort each other, but a search request should not abort a view request probably.

@bastianallgeier
Copy link
Member

You are right, let's keep it simple and specific to each request type.

@distantnative
Copy link
Member Author

@bastianallgeier I am still a bit worried whether this PR is already too far down the chain and canceling out panel requests that shouldn't cancel each other out. What's your thoughts there?

@bastianallgeier
Copy link
Member

It's definitely possible that we miss something here. But to be honest, I really don't know how to properly test that. Maybe we should move this into the alpha? We could also think about introducing a bunch of slow loading models in our lab setup to emulate slow responses in the panel.

@distantnative distantnative marked this pull request as draft September 24, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion 🗣 Requires further discussion to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slower navigation requests still go through even if already navigated elsewhere
2 participants