Skip to content
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

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

Merged
merged 1 commit into from
Jul 24, 2022

Conversation

andersevenrud
Copy link
Member

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 andersevenrud added the enhancement New feature or request label Aug 2, 2021
@andersevenrud andersevenrud force-pushed the feature/localstorage-adapter-ns branch from 7402d2f to c067798 Compare August 2, 2021 20:37
@andersevenrud andersevenrud force-pushed the feature/localstorage-adapter-ns branch 2 times, most recently from b813441 to a378077 Compare August 2, 2021 22:47
@andersevenrud
Copy link
Member Author

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

@andersevenrud andersevenrud force-pushed the feature/localstorage-adapter-ns branch from a378077 to 63a818e Compare August 3, 2021 00:11
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 andersevenrud force-pushed the feature/localstorage-adapter-ns branch from 63a818e to ba523c1 Compare August 3, 2021 00:13
@ajmeese7
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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.

@andersevenrud andersevenrud merged commit cd8add4 into master Jul 24, 2022
@andersevenrud andersevenrud deleted the feature/localstorage-adapter-ns branch July 24, 2022 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants