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

Feature/swr for library screens #186

Merged

Conversation

martinek
Copy link
Contributor

This PR wraps data fetching in library and manga detail screens with SWR which simplifies data management in components and adds benefits of using SWR (mainly client side caching and revalidation).

This also disables automatic online fetching in manga detail, as it currently cannot be done well. As such refresh buttons were added to manga detail so user can trigger online fetch manually.

Tachidesk

Suwayomi/Suwayomi-Server#428 would need to be implemented so that client can decide if it wants to automatically refetch data if it is stale. Currently, it slows down server significantly when user is only browsing library.

This PR is not closing any issue, it is just refactoring and improvement.

I would prefer not to merge this as it is now. It would be better to wait for issue mentioned above to be resolved first and add automatic fetching.

…v and user experience. Refactor some API calls and data handling. Add manual refresh to manga detail.
@AriaMoradi
Copy link
Member

the PR looks good so far,
1- It changes more that one thing, break it into separate PRs
2- The handling of double refresh button is weird (to me and probably users), we should handle it better.

@martinek
Copy link
Contributor Author

martinek commented Oct 29, 2022

1- It changes more that one thing, break it into separate PRs

Ok, I will remove the layout fixes before submitting it.

2- The handling of double refresh button is weird (to me and probably users), we should handle it better.

The refreshing could be automatic (as it is now) once Suwayomi/Suwayomi-Server#431 is merged. But even then, I was thinking of adding some less intrusive information about when the data were last fetched and a way for user to update them manually.

@AriaMoradi
Copy link
Member

The refreshing could be automatic (as it is now) once Suwayomi/Tachidesk-Server#431 is merged. But even then, I was thinking of adding some less intrusive information about when the data were last fetched and a way for user to update them manually.

We could copy what tachiyomi does? @Syer10

@Syer10
Copy link
Contributor

Syer10 commented Oct 29, 2022

Tachiyomi only supports pull to refresh to update manga info and chapters, or using the library updater to update chapters(and manga info if that preference is enabled).

@Syer10
Copy link
Contributor

Syer10 commented Oct 29, 2022

So there is no automatic refresh when a manga is viewed

@AriaMoradi
Copy link
Member

Tachiyomi only supports pull to refresh to update manga info and chapters, or using the library updater to update chapters(and manga info if that preference is enabled).
So there is no automatic refresh when a manga is viewed

We could commit to this change on WebUI with the next server release only if there's proper support for library update

@Syer10
Copy link
Contributor

Syer10 commented Oct 29, 2022

I've cleaned up library update and addressed my issues with it in one of my PRs, so I dont have any problems with using it anymore.

@martinek
Copy link
Contributor Author

We could commit to this change on WebUI

What does that mean exactly? Would there be manual refresh on manga page or not?

@AriaMoradi
Copy link
Member

AriaMoradi commented Oct 30, 2022

We could commit to this change on WebUI

What does that mean exactly? Would there be manual refresh on manga page or not?

manual refresh would reload both the manga and chapters, and we won't worry about the user not having up to date chapters if they miss it because there's a library update button somewhere

@martinek
Copy link
Contributor Author

I have updated the code. There is now only one refresh button and I have added the automatic refresh if data from server are older then 24 hours.

@martinek martinek marked this pull request as ready for review October 31, 2022 16:16
@AriaMoradi
Copy link
Member

I have reviewed the code from different angles and it's fine, only that it brings a paradigm shift with it and we'll need to update the rest of the code similarly.

If all is fine then we can merge.
cc: @Robonau

@martinek
Copy link
Contributor Author

martinek commented Nov 2, 2022

@AriaMoradi I will rewrite rest of the queries in next PR if this PR gets merged.

@AriaMoradi AriaMoradi merged commit 6f8755f into Suwayomi:master Nov 2, 2022
@martinek martinek deleted the feature/swr-for-library-screens branch November 2, 2022 10:40
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.

3 participants