353/user data fetched on page load #1218
Conversation
|
| }); | ||
| setIsAuthenticated(true); | ||
| useEffect(() => { | ||
| localStorage.setItem('userData', JSON.stringify(userData)); |
There was a problem hiding this comment.
@dbradham
why are we using localStorage? isn't data persistent on every navigation to a different page?
There was a problem hiding this comment.
@Enjoy2Live The data is not persistent on every navigation to a different page from my testing. This behavior is confirmed in production. This solution does not address the root issue of files like Navigation.tsx losing their state with every page load. This solution just reduces the amount of requests we are making because of files like that losing their state.
There was a problem hiding this comment.
This sounds like something that can be fixed with the used npm package (constate here).
I've made quick example with the same data set and hook with little modification on it. https://stackblitz.com/edit/stackblitz-starters-ocgwke?ctl=1&embed=1&file=pages%2F_app.js
Taken from constate docs => advanced example
There was a problem hiding this comment.
@Enjoy2Live I'm don't understand what you are recommending to change here.
We are already using constate in this file, but the state is still being reset. Is there something you see that we could improve with our implementation of constate? (I didn't implement the constate code, but it looks correct to me)
There was a problem hiding this comment.
@dbradham Have you looked into the example I provided? it basically fixes the issue of redundant-fetching.
We can standardize the example for teams to use the context this same way the example does
There was a problem hiding this comment.
I can make a PR that fixes this and point it to the PR we're talking about here, I'm not sure what you mean by I can't navigate pages in my example but there's a link that says go to "test" and the other page says go to "home".
These are two separate routes/pages
There was a problem hiding this comment.
@Enjoy2Live Okay, I see. I replaced the changes you made to the context class with the current context class. I am not able to reproduce the redundant requests. (https://stackblitz.com/edit/stackblitz-starters-rilpbv?ctl=1&embed=1&file=pages%2F_app.js)
I'd be happy for you to make this PR, but are you sure this will fix the unwanted behavior in the app? It seems the test environment is fundamentally different than our hosted solution here.
My understanding is that this problem is due to components which call on the UserDataContext having their state reset, not anything within this file. E.g. Navigation.tsx loses all stateful variables with each page load. And this causes the useEffect(() => { ... }, []) to fire again and make the request within the Context class. I don't understand how updating our context class will address this.
There was a problem hiding this comment.
To go back to your original question, data does not seem to be persistent with each page load. Nothing we change in the Context will affect this behavior per my understanding of the root issue.
This is a short-term patch Kris proposed, and I do not have any better solution until the root issue of data not being persistent with each page load across the app. I'm happy to revisit this discussion if you have any insights into the root issue here. Leaving the current implementation and behavior as it is is a risk because it adds lots of extra stress to the backend.
If you would like for me to comment out the use of localStorage so we can test in staging, I will. I'd like to get the rest of these changes merged sooner than later because this change is already in our development/ideaspace branch so it is blocking us from deploying.
There was a problem hiding this comment.
do you think we deploy it to https://ideaspace1.devlaunchers.org to test is a good idea?
I tested it locally anyways by doing yarn workspace @devlaunchers/app build && yarn workspace @devlaunchers/app start but if you want to test it there you can!
This pull request addresses 3 things: