os-js / osjs-client

OS.js Client Module
https://manual.os-js.org/
Other
31 stars 31 forks source link

LocalStorage Settings adapter error #159

Closed oeway closed 2 years ago

oeway commented 3 years ago

The current implementation of LocalStorage Settings adapter assumes all the localstorage values are JSON string (i.e. only produced by OS.js): https://github.com/os-js/osjs-client/blob/baa60b1f9c5f1dc28e9a9d692f572a40d17be58e/src/adapters/settings/localstorage.js#L60

If we have keys that are created outside OS.js which are not JSON string, it fails with this warning:

localStorageAdapter parse error SyntaxError: Unexpected token a in JSON at position 0
    at JSON.parse (<anonymous>)
    at localstorage.js:60
    at Array.reduce (<anonymous>)
    at Object.load (localstorage.js:57)
    at e.value (settings.js:165)
    at Object.load (settings.js:99)
    at e.callback (core.js:238)
    at auth.js:234
andersevenrud commented 3 years ago

But isn't this just a warning ? It should try to parse as JSON and set the value as is if parsing failed.

oeway commented 3 years ago

Yes, but I felt some warning can be suppressed if we assume there can be localstorage values stored by other other app under the same origin. I am not sure exactly what does the localStorageAdapter do, but can it use only a key, or add some prefix (e.g. osjs_) to label all the keys used by osjs?

andersevenrud commented 3 years ago

The localStorage adapter is an adapter for the Settings service which stores user settings. There are alternatives like backend database.

or add some prefix (e.g. osjs_) to label all the keys used by osjs?

Adding a prefix sounds like a good solution for the localStorage adapter, as this is the only one that has this issue.

andersevenrud commented 3 years ago

Opened a PR here: #160

andersevenrud commented 2 years ago

This will be fixed in the upcoming release https://github.com/os-js/osjs-client/pull/160 was added.