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

Avoid error in some environments #638

Merged
merged 1 commit into from
Mar 5, 2018

Conversation

pablobm
Copy link
Contributor

@pablobm pablobm commented Mar 3, 2017

There seems to be a problem, in very specific circumstances, with the code that detects support for session storage:

screen shot 2017-03-03 at 18 36 55

To reproduce follow these steps:

  1. On Google Chrome
  2. Disable 3rd-party cookies
  3. Open the browser inspector
  4. Open on the Ember inspector
  5. Visit a page showing an Ember app, on a frame
    loaded from a different domain

It's important that the Ember inspector is already open when you land on the page (hence step 4 before 5). Reloading the iframe page with the Ember inspector open also reproduces the problem.

This PR appears to fix the problem for me.

Copy link
Contributor

@teddyzeenny teddyzeenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! Should we also set/remove a value like we did for local storage detection?

@pablobm
Copy link
Contributor Author

pablobm commented Mar 4, 2017

Thought of that, but opted for not doing it simply to avoid adding code whose purpose I don't really know. If you think it's worth doing, I can add that.

@pablobm pablobm force-pushed the session-storage-check branch from 20d409e to 3fd6809 Compare March 4, 2018 09:08
@pablobm
Copy link
Contributor Author

pablobm commented Mar 4, 2018

I rebased this today, and the build failed. Looks like three environments timed out before even trying to run the tests. Would it be possible to rerun the build, please?

@rwjblue
Copy link
Member

rwjblue commented Mar 4, 2018

Done!

if (typeof sessionStorage !== 'undefined') {
SESSION_STORAGE_SUPPORTED = true;
}
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to log any sort of message here or does just catching the error retain the functionality, so everything appears good?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching the error retains the functionality, so everything appears good.

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teddyzeenny does this look good to you?

@teddyzeenny teddyzeenny merged commit 86689da into emberjs:master Mar 5, 2018
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