-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Add warning as prevention on drop off of UI from being displayed #2614
Add warning as prevention on drop off of UI from being displayed #2614
Conversation
… is not trying to access the sessionStorage
Deploy preview ready! Built with commit d22ddbe |
Deploy preview ready! Built with commit 50f0ee8 |
Yeah, this looks great! What do you think about putting the same data on the window object if session storage isn't available? So test once when the file is imported if session storage is available and if not, set the same data on something like |
@KyleAMathews yea, that sounds great. Given if we would like to check if the sessionStorage is available or not, do we still keep the try...catch block? |
Yeah, probably safer that way |
} catch (e) { | ||
console.warn(`[gatsby-react-router-scroll] Unable to access sessionStorage; sessionStorage is not available.`) | ||
|
||
if (window && window[GATSBY_ROUTER_SCROLL_STATE] && window[GATSBY_ROUTER_SCROLL_STATE][stateKey]) { |
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.
Did most of the setting of scroll position here because Safari will throw error immediately even if trying to access sessionStorage. Unlike Chrome, Firefox, etc.
try { | ||
sessionStorage.setItem(stateKey, storedValue) | ||
} catch (e) { | ||
if (window && window[GATSBY_ROUTER_SCROLL_STATE]) { |
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.
Same goes here too. Did the saving of the scroll position here, rather than having the check in try block because browser like Safari would throw error immediately, even when just trying to check using window.sessionStorage
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.
Yeah, this is totally reasonable way of doing things. It seems it's quite rare for this error to show up as there's been lots of Gatsby sites built without me hearing about this until now :-)
So even if a try/catch is perhaps a bit slower, it's a very rare code path.
@KyleAMathews I've added on having to set the scroll position in I still preserve the Let me know what you think about it 😃 |
Thanks! |
Awesome 😄 No probs. Actually, thank you for giving me the opportunity fix it 👍 |
Hiya @emmafallancy! 👋 This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here. Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! 💪💜 |
Add a simple prevention to avoid any drop off on UI from not being displayed due to the attempt of trying to read or write into sessionStorage when sessionStorage itself is not available (e.g. disabling cookies, security reason, etc.)
At least having this fix would be not be blocking from the UI being displayed. This would mainly happened when cookies disabled (in most scenario).
@KyleAMathews let me know what you think about gatsby-react-router-scroll fix 😄