ScoreSaber / scoresaber-frontend

MIT License
87 stars 16 forks source link

Duplicated API calls #49

Closed motzel closed 2 years ago

motzel commented 2 years ago

Player profile:

image

Leaderboards:

image

Rankings:

image

This is related to the useAccio() thingy. For example on player profile it's loaded in onMount()

https://github.com/ScoreSaber/ScoreSaber-Frontend/blob/9cf717531ce9bae671c20667b0b74b307bdc4fce/src/routes/u/%5BplayerId%5D.svelte#L56-L66

https://github.com/ScoreSaber/ScoreSaber-Frontend/blob/9cf717531ce9bae671c20667b0b74b307bdc4fce/src/lib/utils/accio/index.ts#L16-L18

and then the reactive page store comes into play:

https://github.com/ScoreSaber/ScoreSaber-Frontend/blob/9cf717531ce9bae671c20667b0b74b307bdc4fce/src/routes/u/%5BplayerId%5D.svelte#L68-L75

...and the data is loaded again.

One of potential solutions is add new field like loadOnMount : boolean to the AccioOptions

https://github.com/ScoreSaber/ScoreSaber-Frontend/blob/9cf717531ce9bae671c20667b0b74b307bdc4fce/src/lib/utils/accio/index.ts#L163-L170

and setting it only where it is actually needed (/api/user@me endpoint, sth else?).

Umbranoxio commented 2 years ago

Oh wow, excellent catch. We didn't have the reactive page store functionality until recently so this kind of slipped through undetected, fixing it right now

Umbranoxio commented 2 years ago

Every way I've tried solving this has failed so while I'm fixing up parts of the API I'm going to have to take myself off this issue

motzel commented 2 years ago

I can fix it if @Dannypoke03 doesn't have time/is busy with something else. I had previously done a POC as written earlier and that solved the issue. I didn't make a PR because I didn't know if you want to solve it this way or the other.

Dannypoke03 commented 2 years ago

Had some time to work on it earlier today, just made it a daft PR now #52, if you've got ideas that I missed happy to discuss them over on that

motzel commented 2 years ago

Your implementation of checking the current request in progress is great.

I have one more idea to further optimize the number of API requests. When a player's profile is accessed, the profile data is retrieved on the server side.

https://github.com/ScoreSaber/ScoreSaber-Frontend/blob/b1b59a8c238568d834cd6f92b503548416300836/src/routes/u/%5BplayerId%5D.svelte#L1-L7

But then it is only used as page metadata:

https://github.com/ScoreSaber/ScoreSaber-Frontend/blob/b1b59a8c238568d834cd6f92b503548416300836/src/routes/u/%5BplayerId%5D.svelte#L158-L172

And it could also be passed as initialData to useAccio() and then the client side could skip the first request altogether. What are your thoughts on this?

I have no prior experience with SveteKit, so I don't know if such partial hydration is possible, though.

motzel commented 2 years ago

I played around with it a bit and yes, it works.

image

Dannypoke03 commented 2 years ago

Yeah, I'm not surprised that partial hydration is supported to some extent, and yeah that's a rather smart idea to use the metadata request to hydrate some of the pages as well. I'll have some more free time tomorrow to look at an elegant solution to get some partial hydration working on the profile page, but also on other pages where metadata is being loaded in.

Umbranoxio commented 2 years ago

The problem with using metadata to hydrate is the metadata request doesn't contain the users session, which right now would mean admins and ranking team see a different version of the data, and could effect more things going forward

Umbranoxio commented 2 years ago

This is solvable and svelte kit can be fully SSR, but I'm not sure if it's worth it in this case