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

#356 / #330 Bug: reduce page load times & look into useCallback #363

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

Conversation

chris-nowicki
Copy link
Contributor

fixes #330 and #356

  • Reduced page load time by 2.5 seconds by removing unneeded useEffects
  • Looked into the useCallback hook for getUser() in AuthContextProvider and found that it was not required

SCREENSHOT

CleanShot 2024-07-02 at 14 36 09

VIDEO

CleanShot.2024-07-02.at.14.14.37-converted.mp4

Copy link

vercel bot commented Jul 2, 2024

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

Name Status Preview Comments Updated (UTC)
gridiron-survivor ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2024 3:37am
gridiron-survivor-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2024 3:37am

Copy link

appwrite bot commented Jul 2, 2024

Gridiron Survivor Application 6616ea581ef9f5521c7d

Function ID Status Action
Your function has been successfully deployed.

Project name: Gridiron Survivor Application
Project ID: 6616ea581ef9f5521c7d

Function ID Status Action
userAuth 6626fef885a9f630442b ready Ready View Logs

Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.

💡 Did you know?
Cursor pagination performs better than offset pagination when loading further pages

Copy link
Collaborator

@shashilo shashilo left a comment

Choose a reason for hiding this comment

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

Added a comment, but we also need a unit test to test when the code calls getUser().

@@ -18,6 +19,11 @@ const Leagues = (): JSX.Element => {
const [leagues, setLeagues] = useState<ILeague[]>([]);
const [currentWeek, setCurrentWeek] = useState<IGameWeek['week']>(1);
const { user } = useDataStore((state) => state);
const { getUser } = useAuthContext();

if (!user.id || user.id === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go into the AuthContextProvider. Otherwise, every page will need this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Done.

@@ -42,10 +42,6 @@ const Leagues = (): JSX.Element => {
};

useEffect(() => {
if (!user.id || user.id === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This entire empty user will be fixed with this change? I had to add it to 2 other pages because that's how it works for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashilo I am confused. need more info. I fixed this to remove it from page.tsx and moved that into the useEffect of authContextProvider as that is the only place I could get it to work and the provider wraps around all pages.

*/
const getUser = useCallback(async () => {
const getUser = async (): Promise<void | IUser> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will need unit tests for this file. I do not see a test file at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

roger that. I will add one.

@shashilo
Copy link
Collaborator

shashilo commented Jul 7, 2024

@chris-nowicki You'll have to review this again. This is still rendering multiple times because of the way your context is set up. LMK when you have time, I'll show you how to debug this.

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

Successfully merging this pull request may close these issues.

Bug: Check useCallback function to make sure it is needed
2 participants