Skip to content

Cannot Stub localStorage in Firefox 35 #662

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

Closed
jherdman opened this issue Jan 25, 2015 · 18 comments
Closed

Cannot Stub localStorage in Firefox 35 #662

jherdman opened this issue Jan 25, 2015 · 18 comments
Labels

Comments

@jherdman
Copy link

It seems localStorage cannot be stubbed with Sinon 1.12.2 in Firefox 35. Here's a JSBin that illustrates this problem http://jsbin.com/nepehiriga/1/edit?js,output.

Possibly helpful jasmine/jasmine#299

@mroderick
Copy link
Member

This seems related to #306

In past projects, we've created wrappers around native objects, including localStorage exactly because of these limitations.

Perhaps someone should put out a little library that we can point to ...

@mroderick
Copy link
Member

@jherdman have you been able to do this with other combinations of Sinon.JS and Firefox?

@jherdman
Copy link
Author

jherdman commented Feb 8, 2015

Hi! I've verified that this problem still exists in FF 37.0a2 (current version of Developer Edition as of writing this reply).

@jherdman
Copy link
Author

jherdman commented Feb 8, 2015

In the past I've used things like amplify.js's localStorage wrapper, but this feels really silly given that localStorage is so broadly supported (http://caniuse.com/#search=localStorage).

If the problem cannot be fixed directly, perhaps sinon should throw an error when it's asked to stub such an object's methods?

@mroderick
Copy link
Member

In the past I've used things like amplify.js's localStorage wrapper, but this feels really silly given that localStorage is so broadly supported (http://caniuse.com/#search=localStorage).

It makes good sense to me to wrap apis, so it's easier to test things. It also gives you an opportunity to adjust the api to include mandatory defaults, handle weird errors in a single place, or just make your code clearer by eliminating duplication.

I have no problem wrapping builtin api's, that's what we've been doing with jQuery and the DOM for almost a decade now.

If the problem cannot be fixed directly, perhaps sinon should throw an error when it's asked to stub such an object's methods?

I can imagine one way to do it, would be to have a quick feature test to set up some flags, and then in the stub function, throw an error if it's called with localStorage.

If we decide to take this route, we'll have to add it for pretty much every API that is implemented in browsers, or it's not going to be surprising that Sinon warns you about some APIs, and not others.

I think down that road lies a maintenance nightmare.

How would you implement this?

@mroderick
Copy link
Member

store.js looks like a promising wrapper for localStorage

@jherdman
Copy link
Author

jherdman commented Feb 8, 2015

I wonder if we can leverage Object.getOwnPropertyDescriptor? Here's a sample session in Firefox's developer console:

screen shot 2015-02-08 at 1 06 18 pm
screen shot 2015-02-08 at 1 06 00 pm

And in Chrome:

screen shot 2015-02-08 at 1 06 36 pm
screen shot 2015-02-08 at 1 06 52 pm

It may be reasonable to assume that anything that doesn't have a property descriptor isn't safe to stub and a warning should be issued. This is obviously not entirely true given that I can safely stub localStorage in Chrome and Phantom...

@mroderick
Copy link
Member

Interesting idea to use Object.getOwnPropertyDescriptor, but easily defeated.

var Car = function() {};
Car.prototype.getItem = function() {};
var car = new Car();
Object.getOwnPropertyDescriptor(car, 'getItem');
// undefined

@mroderick
Copy link
Member

Oh, there's also lockr

@jherdman
Copy link
Author

Dang! That's heartbreaking. I think it's reasonable to close this issue given that there's no obvious solution to the problem. At least with this issue hopefully future trouble sinon users will see a potential solution: wrap the offending library, and silently sob that Object.getOwnPropertyDescriptor doesn't always work as we'd like.

@mroderick
Copy link
Member

I'd like to write some documentation on the matter, or a couple of blog posts about the merits of wrapping apis... one of which would be: "make stubbing possible"

@irvingreid
Copy link

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1141698 to see if we can get Firefox to add support.

@bzbarsky
Copy link

I just marked that bug as invalid, because the Firefox behavior here is the one required by the spec for localStorage and sessionStorage, due to the named setter on them. Yes, named setters are insane.

The right fix is to modify the functions on Storage.prototype, which is where they live, instead of modifying them on the instance objects...

@bzbarsky
Copy link

The other possible fix is to try to get the spec for localStorage and sessionStorage changed in some way, I guess.

@jherdman
Copy link
Author

Two quick questions for a relative newb to modern JS, hopefully one of you smart folks can point me to some helpers:

  • What's a named setter?
  • Where do I find the docs for Storage and its relationship to localStorage/sessionStorage?

@bzbarsky
Copy link

What's a named setter?

It's something that allows things that look like just a basic property set to get turned into a method call on the object. In the case of session/localStorage, it gets turned into setItem. If you want more details, there's spec text at http://heycam.github.io/webidl/#idl-named-properties and http://heycam.github.io/webidl/#indexed-and-named-properties but it's not the easiest thing in the world to follow. Plus you have to mentally apply the changes from whatwg/webidl#45.

Where do I find the docs for Storage and its relationship to localStorage/sessionStorage?

Storage is the interface those objects implement. See https://html.spec.whatwg.org/multipage/webstorage.html#the-sessionstorage-attribute and https://html.spec.whatwg.org/multipage/webstorage.html#the-localstorage-attribute and https://html.spec.whatwg.org/multipage/webstorage.html#the-storage-interface

@mroderick
Copy link
Member

@jherdman I've created a tiny wrapping module that you can use as a stubbing target

https://github.com/mroderick/wrapple

Can this ticket be closed?

@mroderick
Copy link
Member

@jherdman It's been ~1 month, I am closing this. Re-open it with more details, if you feel there's something we haven't covered.

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

No branches or pull requests

4 participants