-
-
Notifications
You must be signed in to change notification settings - Fork 12
fix: Adding missing field to store #101
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
Conversation
✅ Deploy Preview for eslint-code-explorer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
const storedValue = searchParams.get(key) ?? ""; | ||
const storedValue = getHashParams().get(key) ?? ""; | ||
return storedValue ? JSON.parse(atob(storedValue)) : ""; | ||
}, | ||
setItem: (key, newValue): void => { | ||
const searchParams = new URLSearchParams(location.hash.slice(1)); | ||
const searchParams = getHashParams(); | ||
const encodedValue = btoa(JSON.stringify(newValue)); | ||
searchParams.set(key, encodedValue); | ||
location.hash = searchParams.toString(); | ||
}, | ||
removeItem: (key): void => { | ||
const searchParams = new URLSearchParams(location.hash.slice(1)); | ||
const searchParams = getHashParams(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like we're creating a new URLSearchParams
for each method. Is that necessary or can the same one be reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is necessary to create a new URLSearchParams. If we reused a single URLSearchParams instance, it would contain stale data that doesn't reflect the current state of location.hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like another review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks!
@amareshsm should we remove the dropdown change case then? I can send a PR for that? |
Prerequisites checklist
What is the purpose of this pull request?
What changes did you make? (Give an overview)
Use the
onRehydrateStorage
hook to add any new field if it is missing in the store.The previous implementation - #97, where we checked for missing language code, works fine in cases where a user manually selects a new language from the dropdown. In this scenario, if the corresponding code entry was missing, we could dynamically patch it on the fly.
However, this approach breaks when a user opens a shared link that points directly to a newly added language. Since the state is rehydrated from the URL hash and doesn’t include the new language (because it wasn’t present in the original persisted state), the app can break.
We’re now using the onRehydrateStorage hook provided by Zustand. This hook ensures that immediately after the state is loaded from storage, any missing fields (like code for a new language) are automatically added using defaultCode. This centralises the patching logic inside the store (use-explorer file), ensures the state is always complete, and prevents issues with incomplete or outdated persisted data.
Also, all code logic related to state hydration and defaults stays cleanly inside the Zustand store.
Related Issues
Is there anything you'd like reviewers to focus on?