-
Notifications
You must be signed in to change notification settings - Fork 1
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
Session storage is poorly handled #249
Comments
Is there a reason that the sessionStorage is not handled with redux or some other global state? Or has it just not been done yet™ I have been trying to wrap my head around how it could be preferable to use sessionStorage so it is persistent between reloads instead of populating a global state from the backend on page load, but so far that seems preferable - and would syncronize the data more frequently. |
The frontend was quite small at the beginning (and actually is quite small now as well), therefore redux was not used. Context and Hooks were not a thing at the time either, so everything was just done with props. The reason we have the session storage is to remove the danger of someone writing large applications and losing everything due to a refresh. But, as the issue entails, the whole thing was an afterthought. We manically patched and extended it multiple times right before the admissions in order to fix bugs. The result is a buggy and sub-optimal storage solution.
If we only populate and stored the data on the backend we would either 1) have to callAPI with the latest application details I don't know if I understood you correctly, but is your proposal to scrap the whole sessionStorage, and only push and fetch changes from the backend? I am open to any change/rewrite that makes it better 👍🏻 |
Ah right, I could've worded that more clearly. Got to thinking about the risk of losing data on reload a bit after writing that initial comment. My proposal is to scrap sessionStorage, and replace it with localStorage in the ApplicationPage. There it should only be used to keep track of the text you have written and not yet submitted. That way not yet completed applications would not be deleted if the tab is closed on accident (which it will be with sessionStorage), and if the application is not in the backend any more and only in local/sessionStorage it will not appear as if it is when the user opens the site. localStorage should probably have some sort of timestamp to ignored after some time, but it would create a more reliable draft option. Thinking about it the localStorage part of the proposal is not that important to me, but I want sessionStorage gone from already submitted applications and only to function as a draft option.. |
I like it!
This is the important part. It needs a little rewrite for the fetching/storing logic. As Ludvig proposed, the application should be fetched once in a top-level component where it's stored in a global state (Context maybe), then managed in a more clear way where only the draft part is stored and later submitted. |
You have several options here for storing data in a better way (redux is not a good way!) Some of these (react query & SWR) I really like cause they "just work" but they don't provide much manual control. They are more "data fetching with caching and invalidation + automatic refetching" than they are state management. I do like the proposal of having the draft stored in the db. In that case (i.e. all state is managed in the backend) the data fetching state managers are perfect. I do also agree that this method is also much simpler and robust, since we don't need logic to handle when and what to sync and how we store the data in the store etc. In this case I would just have a simple hook to update the form in the backend and put it on a debouce to just store the latest data in the backend when it makes sense to do so. And just scrap any localstorage or similar. So more logic in the backend is required, but that's essentially just a |
Session storage sync with form data is poorly handled as is. This leads to quite a few issues, such as the field data and storage not being synced properly, and sometimes both are overwritten.
Proposal
The session storage should be retrieved once in a top level component. And possibly set when fetching an admission.
When editing, the session storage should be set in
onChange
handlers in each field. (and not with setTimeouts...)The text was updated successfully, but these errors were encountered: