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

Switch to getLiteUser where applicable on frontend #4951

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

nabramow
Copy link
Contributor

@nabramow nabramow commented Oct 6, 2024

This PR switches out useUser and useUsers with useLiteUser and useLiteUsers, which returns a much smaller user object. Backend code was already merged for this.

HOW TO TEST

  • Check anywhere that it getLiteUser or getLiteUsers effects. This is anywhere you just have user avatar, name, etc (almost everywhere except profile and map). So references, chat, comments on communities and events, friend requests, host requests, etc.

Closes #4936

Web frontend checklist

  • [ x ] Formatted my code with yarn format
  • [ x ] There are no warnings from yarn lint --fix
  • [ x ] There are no console warnings when running the app
  • [ N/A ] Added any new components to storybook
  • [ N/A ] Added tests where relevant
  • [ x ] All tests pass
  • [ x ] Clicked around my changes running locally and it works
  • [ N/A ] Checked Desktop, Mobile and Tablet screen sizes

Copy link

vercel bot commented Oct 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
couchers ✅ Ready (Inspect) Visit Preview Oct 26, 2024 6:16pm

@bakeiro
Copy link
Contributor

bakeiro commented Oct 6, 2024

wow such a nice job, looking good!

@bakeiro
Copy link
Contributor

bakeiro commented Oct 6, 2024

I'll test to see if everything works good on the page

@nabramow
Copy link
Contributor Author

nabramow commented Oct 14, 2024

@aapeliv @bakeiro just checking up on this one. I did click around the app already for this but if anyone wants to take a second look I couldn't check the reference user info as I don't have any references in my test account (and can't add as I'm blocked by not having a profile photo and doesn't let me upload one on localdev).

Although we had pretty good tests for most of these components so hopefully it would catch anything.

@bakeiro
Copy link
Contributor

bakeiro commented Oct 15, 2024

sorry, totally forget, if the test was good then is a ready to go for me, I´ll check it briefly, if all good I approve it :)

@nabramow
Copy link
Contributor Author

@bakeiro @aapeliv Okay whoops realizing I missed a big part of this by not using the GetLiteUsers new api so give me some time to adjust the hooks and re-write the tests!

@nabramow nabramow changed the title Switch to getLiteUser where applicable on frontend Switch to getLiteUser where applicable on frontend [WIP] Oct 16, 2024
@nabramow nabramow marked this pull request as draft October 16, 2024 03:59
@darrenvong
Copy link
Member

This is exciting to see - the original useUsers hook implementation has been a hack all along to work around the limitation of not being able to get multiple users in one trip! I'd go as far to say a good measure of this new API doing its work is that there should be no useEffects and the weird workaround with the ref to store the list of user IDs once you've fully switched over to the new RPC!

@nabramow nabramow changed the title Switch to getLiteUser where applicable on frontend [WIP] Switch to getLiteUser where applicable on frontend Oct 25, 2024
@nabramow nabramow marked this pull request as ready for review October 25, 2024 22:35
@nabramow
Copy link
Contributor Author

This is exciting to see - the original useUsers hook implementation has been a hack all along to work around the limitation of not being able to get multiple users in one trip! I'd go as far to say a good measure of this new API doing its work is that there should be no useEffects and the weird workaround with the ref to store the list of user IDs once you've fully switched over to the new RPC!

Thanks for this, I was getting hung up on refactoring the existing hook but this made me realize I should just re-write and use my own common sense haha, since the whole api call works differently.

Done now, re-wrote the tests. Could be missing some test cases, but I have most of the old ones covered (except the invalidate param since we don't need to pass that anymore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Use LiteUser in lots of places in the frontend
3 participants