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

Refactor: Use a global AbortController #1162

Merged
merged 2 commits into from Sep 1, 2022

Conversation

marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Aug 24, 2022

The rationale for this is that request code currently lives in components without any way to keep the code DRY.
Ideally, request code would live in vuex actions IMO, but then we lose the ability to cancel requests from views.
This PR removes logic from views that handles request cancellation and moves it to the router, which cancels requests upon route changes. I've also introduced a global priority queue for requests to replace the semaphore business, which also would have to be added to every request otherwise.

Copy link
Collaborator

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Overall, I think only requests that fetch photos or albums should be canceled. They are read only requests and the only ones that can take a long time to load. The other ones are here to alter the DB, so they should not be canceled.

I would prefer for the views to keep the responsibility to cancel the requests, and not a central service. Using a mixin here might make more sense.

src/store/albums.js Outdated Show resolved Hide resolved
src/views/AlbumContent.vue Show resolved Hide resolved
@marcelklehr
Copy link
Member Author

marcelklehr commented Aug 24, 2022

Using a mixin here might make more sense.

Mh, but then wouldn't we need a mixin and a cancelRequest property for every request we do? Because every request gets its own cancelRequest property to cancel on view destruction. It's a lot of manual book keeping.

Update: Or do you mean putting only the cancelAll() call into a mixin, so views can decide whether they want to use it?

@marcelklehr marcelklehr changed the title Refactor: Use a global AbortController to cancel requests on route changes Refactor: Use a global AbortController and PQueue Aug 24, 2022
@marcelklehr marcelklehr force-pushed the refactor/request-autocancel-queue branch 2 times, most recently from 141386e to ca0aef4 Compare August 26, 2022 09:50
@marcelklehr marcelklehr changed the title Refactor: Use a global AbortController and PQueue Refactor: Use a global AbortController Aug 26, 2022
src/router/index.js Outdated Show resolved Hide resolved
@marcelklehr marcelklehr mentioned this pull request Aug 30, 2022
@artonge
Copy link
Collaborator

artonge commented Sep 1, 2022

/compile amend /

…anges

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr marcelklehr force-pushed the refactor/request-autocancel-queue branch from d5e5909 to 02e9750 Compare September 1, 2022 11:01
@marcelklehr
Copy link
Member Author

/compile amend /

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@marcelklehr marcelklehr merged commit 7134e18 into master Sep 1, 2022
@marcelklehr marcelklehr deleted the refactor/request-autocancel-queue branch September 1, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants