search

os-js / osjs-client

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

Add prefix to localStorage settings adapter (#159) #160

Closed andersevenrud closed 2 years ago

andersevenrud commented 3 years ago

Avoid parsing of localStorage entries that does not contains a configured prefix because it could contain data that has been set in other applications under the same host.

andersevenrud commented 3 years ago

Opsie. Just realized I stashed the save method changes because I was switching branches. Applied :blush:

ajmeese7 commented 2 years ago

Is this and #159 still in need of any assistance? I've made some modifications to my fork of the localStorage adapter so I feel pretty comfortable with it if you need me to look at/add anything. About to go on vacation but if so I will check back when I return home.

andersevenrud commented 2 years ago

I can't remember working on this... but since it's still open I assume that it's not finished, so any help is appreciated @ajmeese7 .

ajmeese7 commented 2 years ago

I just tried this out in my implementation of the library and it seems to work exactly as intended. I added some additional documentation to my localstorage.js file if you are interested in that, but other than that I'm not sure what there is left to implement for this feature. The tests pass as well.

andersevenrud commented 2 years ago

The only thing that has not been solved is some kind of migration support so that settings are not lost with an update.

But not really sure that's really needed. Maybe just a notice in the release notes will do.

ajmeese7 commented 2 years ago

I agree, if the user requires that the old localStorage items be respected, they can simply make the prefix empty or remove it entirely so things will continue to work as they are now. This is more of an edge case feature anyways that I don't see the average user struggling with much.

The release notes mention should be a perfect solution to this.