Suwayomi / Suwayomi-WebUI

Mozilla Public License 2.0
107 stars 50 forks source link

refactor(tracker): changes the logic of calling the TRACKER_FETCH_BIND to the onclick of the tracker button to follow mihon behavior #695

Closed taos15 closed 5 months ago

taos15 commented 5 months ago

This pull request refactors the logic of calling the TRACKER_FETCH_BIND to the onclick of the tracker button to follow mihon behavior. It also updates the useEffect hook to fetch track info on component mount.

schroda commented 5 months ago

these changes will make it fetch everytime the manga page gets opened and then again when clicking the track button in the manga page

what is wrong with the current way it's done, it gets called when the track information are actually showed, same way it's done in mihon?

taos15 commented 5 months ago

We can be removed the useEffect in the details page and leave only the on-click handler. Making the refresh on-click instead of the useEffect on the model makes it less likely to bugs or multiple fetch. You can see with the strict mode that it makes 6 request instead of 3. Since you want to refresh when you click on the tracker button it makes more sense to add the logic in the on-click event instead of relying on the state.

schroda commented 5 months ago

I still don't get whats wrong with the current logic?

It gets refreshed when the active card of an track binding is shown, which only happens if you open the track component for a manga the component gets rendered for a track record and will trigger a refresh, what is there to cause a bug?

yes, that happens only in strict mode, which is intended by design?

removing the fetch from the component will break the refreshing logic when the component gets opened via the library. to fix that, you have to copy&paste the refresh trigger to that place as well instead of having it one place

taos15 commented 5 months ago

You are right, there is nothing wrong with the current logic. There is not problem until there is a problem, so even if there is not clean up in the useEffect, there should be not problem either the current implementation.