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

Add warning as prevention on drop off of UI from being displayed #2614

Merged

Conversation

efallancy
Copy link
Contributor

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 😄

@gatsbybot
Copy link
Collaborator

Deploy preview ready!

Built with commit d22ddbe

https://deploy-preview-2614--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Oct 25, 2017

Deploy preview ready!

Built with commit 50f0ee8

https://deploy-preview-2614--gatsbygram.netlify.com

@efallancy efallancy changed the title Add console warning as prevention on drop off of UI from being displayed Add warning as prevention on drop off of UI from being displayed Oct 25, 2017
@KyleAMathews
Copy link
Contributor

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 window.___GATSBY_REACT_ROUTER_SCROLL

@efallancy
Copy link
Contributor Author

@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?

@KyleAMathews
Copy link
Contributor

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]) {
Copy link
Contributor Author

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]) {
Copy link
Contributor Author

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

Copy link
Contributor

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.

@efallancy
Copy link
Contributor Author

efallancy commented Oct 26, 2017

@KyleAMathews I've added on having to set the scroll position in window.___GATSBY_REACT_ROUTER_SCROLL when it is not possible to access the sessionStorage. Also, I did most of it in catch block, due to browser like Safari. Whenever trying to access sessionStorage, even using window.sessionStorage, it will immediately throw exception, unlike Chrome, Firefox, etc.

I still preserve the warning message, just in case. Let me know if you consider on having to remove it or altering it to different message.

Let me know what you think about it 😃

@KyleAMathews
Copy link
Contributor

Thanks!

@KyleAMathews KyleAMathews merged commit 361485b into gatsbyjs:master Oct 26, 2017
@efallancy
Copy link
Contributor Author

Awesome 😄 No probs. Actually, thank you for giving me the opportunity fix it 👍

@jlengstorf
Copy link
Contributor

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:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (We’ve got t-shirts and hats, plus some socks that are really razzing our berries right now.)
  2. If you’re not already part of it, we just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again! 💪💜

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

Successfully merging this pull request may close these issues.

4 participants