grevory / angular-local-storage

An AngularJS module that gives you access to the browsers local storage with cookie fallback
Other
2.83k stars 585 forks source link

.get() cannot return stored non-JSON values #230

Open tbassetto opened 9 years ago

tbassetto commented 9 years ago

I have tried to upgrade from version 0.15 to 0.2.1, but the latest version cannot read value stored with the previous one.

We store a token by using:

localStorageService.set('token', newToken); // newToken is just alphanumeric characters

And now the following code is crashing:

localStorageService.get('token'); // token is just alphanumeric characters

I had a look at the code and the issue is that now you always apply JSON.parse() while before you used to call fromJson() only when it made sense.

Is it possible to revert to the previous behavior? Why has this change been made?

btesser commented 9 years ago

I have the same problem. I tracked it down to this code 0.2.0

    if (isUndefined(value)) {
        value = null;
  } else if (isObject(value) || isArray(value) || isNumber(+value || value)) {
        value = toJson(value);
      }

0.2.1

if (isUndefined(value)) {
        value = null;
      } else {
        value = toJson(value);
      }
a8m commented 9 years ago

Can you please provide some example @tbassetto ? Please take a look on #225, #222, #214, etc.. if you want to get some point why this changes made for.

Thanks.

btesser commented 9 years ago

This sounds like a breaking change that needs to be noted in the changelog. for example: these key value paris cause an error image

btesser commented 9 years ago

@a8m please look at examples from previous comment

joecode commented 9 years ago

thanks @tbassetto for pointing out the root of the problem and agree with @btesser. while this change certainly makes sense, there should be alarm bells. it got deployed in production before we caught it and now i have to monkey wrench things back into shape with stuff like:

if (/^[a-zA-Z0-9]*$/.test(token)) {
    console.log('jsonifying token');
    localStorage['ls.token']=JSON.stringify(token);
}

(of course that only handles the localStorage version, not the cookie based fallback.)

a8m commented 9 years ago

Fixed via v0.2.2. Can you please validate that this issue has been resolved?

Thanks.

joecode commented 9 years ago

Doesn't matter for my use case, but if someone was storing quoted strings based on the old setter function, wouldn't this inadvertently remove them? I.e. imagine "This is a famous quote" stored verbatim as as string.

Seems like a difficult problem to fully solve because there's no telling how people may have used (or abused?) the original verbatim string functionality.

stefanhettich commented 9 years ago

Still not working for me, because the try-catch doesn't seem to work the way it should. Debugged your code a bit and found following solution for me:

Your old code:

try {
   return JSON.parse(item);
} catch (e) {
   return item;
}

My fix:

try {
   a = JSON.parse(item);
   return a;
} catch (e) {
   return item;
}

Because of this the try-catch now seems to work!

But the set-method has still a problem if a simple string is passed: It transforms the string "teststring" to "\"teststring\"" (see comment from @btesser)