arielsalminen / feature.js

Feature.js is a fast, simple and lightweight browser feature detection library in 1kb.
https://featurejs.com/
3.17k stars 115 forks source link

Rewrite localStorage Test #64

Closed meowsus closed 5 years ago

meowsus commented 5 years ago

Testing for the presence of localStorage on window (as per #21) before trying to set an item is preferable. The test was rewritten to test the return value of localStorage.getItem rather than setItem since we should be making sure we are getting the values back correctly.

Resolves #21

meowsus commented 5 years ago

@ShirtlessKirk is this what you were thinking?

@viljamis I'm not convinced that we actually need the try..catch now. Do you agree that it can be removed?

I did some testing around this, especially in Private mode, and this test still passes since setItem was able to operate as expected, but my testing was limited and quick. The main point of this PR is to test the actual return value of getItem which I feel is a much safer test.

meowsus commented 5 years ago

@viljamis Also this is technically breaking, but not so much so that I don't think it can get pushed into the next minor.

ShirtlessKirk commented 5 years ago

@meowsus sorry for late reply.

Yep, something like that.

Thing is, what I said way back when isn't as true now: Private Browsing no longer throws an error when setting an item (because the quota was set to 0); there's now parity with Chrome in that regard. This is why the try ... catch appear superfluous, however QUOTA_EXCEEDED could still be triggered due to other data filling up the storage.

I also missed out on saying that localStorage in Incognito mode operates the same as sessionStorage; anything set is discarded on (last window / tab open with that site) close.

So now we have a different (reduced) set of possibilities:

Therefore the code could be changed to almost what was there before, just with a couple of tweaks:


localStorage: (function () {
  if ('localStorage' in window) {
    try {
      window.localStorage.setItem('featurejs-test', 'foobar');
      window.localStorage.removeItem('featurejs-test');
    } catch (err) {
      // no content in the cache means it couldn't be added to at all (old Safari)
      // otherwise we just went over a non-zero quota
      return !!window.localStorage.length;
    }
  }

  return false;
})(),