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

Update implementation #1

Closed
wants to merge 1 commit into from
Closed

Update implementation #1

wants to merge 1 commit into from

Conversation

dguo
Copy link

@dguo dguo commented Aug 20, 2017

This update takes care of the situation where the storage is supported
by the browser, but it's not actually available.

See a more in-depth explanation and updated implementation at https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API/Using_the_Web_Storage_API#Feature-detecting_localStorage

A reason not to merge this would be that someone might only want to know if the feature is technically supported and doesn't care if it's actually usable. It's hard for me to imagine such a case, but it's possible.

This update takes care of the situation where the storage is supported
by the browser, but it's not actually available.
@Download
Copy link
Owner

I wrote that actual MDN section.

A reason not to merge this would be that someone might only want to know if the feature is technically supported and doesn't care if it's actually usable.

The current implementation proves it is usable and returns false in all other cases.

The updated implementation would return true in some circumstances even after setting and removing a key actually failed. So the statement that it more clearly indicates whether storage is actually usable is false. In fact it might return true even though writing a key failed.

The first lines of the code are getting the storage object from the window and calling setItem on it. These lines might actually already fail with exceptions before even attempting the write, because Firefox will throw exceptions in some cases when you access the storage object (without even calling a method on it yet). IE will in some case give you a null object which would cause the method call to fail as well. So you can get other types of exceptions in the catch block than just quota exceeded exceptions. So I simply do not agree with these changes. I think they are wrong.

@Download Download closed this Jul 26, 2018
@Download
Copy link
Owner

Thanks anyway, I do appreciate you are trying to contribute. But people on MDN keep changing a perfectly fine code example and in many cases are actively breaking it in the process so I sort of gave up on trying to maintain the MDN docs.

@dguo
Copy link
Author

dguo commented Jul 26, 2018

Roger that. Thanks for the detailed explanation.

@dguo dguo deleted the patch-1 branch August 9, 2018 15:45
@dguo dguo restored the patch-1 branch August 9, 2018 15:45
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.

None yet

2 participants