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

feat: allow to configure KEY_PREFIX #14

Closed
bogdbo opened this issue Oct 11, 2018 · 11 comments · Fixed by #15
Closed

feat: allow to configure KEY_PREFIX #14

bogdbo opened this issue Oct 11, 2018 · 11 comments · Fixed by #15
Labels

Comments

@bogdbo
Copy link

bogdbo commented Oct 11, 2018

Hello,

I'm talking about this file line 9 and 58
https://github.com/ionic-team/capacitor/blob/master/core/src/web/storage.ts

We already have an existing application and the current Web version of Storage will not read the existing values from local storage due to this KEY_PREFIX.

I can see this is being used for clear() and keys() etc but right now it's breaking existing applications. I guess we can force KEY_PREFIX to an empty string for now but this should be improved in my opinion.

Thank you!

@jcesarmobile
Copy link
Member

The plugin is a wrapper around localStorage, so if you have data already in localStorage you will need to migrate it by checking current data, add it as Capacitor data and remove you copy of localStorage, or keep using localStorage as you already do. The only problem with that is it won't use the native implementation on iOS and Android.

Maybe we can add a migrate function for web implementation that does it for you, what do you thing @mlynch?

@Nasicus
Copy link

Nasicus commented Oct 15, 2018

@jcesarmobile In my opinion it should be possible to set a custom KEY_PREFIX (obviously it can unofficially be done, by just setting KEY_PREFIX now, but it's not nice...). This would not only allow to seamlessly solve situations like this, but also to have capacitor/plugin independent keys (without the prefix).

@jcesarmobile jcesarmobile changed the title KEY_PREFIX of web implementation of 'Storage' breaks existing app allow to configure KEY_PREFIX of web implementation of 'Storage' Jan 9, 2020
@jcesarmobile jcesarmobile changed the title allow to configure KEY_PREFIX of web implementation of 'Storage' feat: allow to configure KEY_PREFIX of web implementation of 'Storage' Jan 9, 2020
@jcesarmobile
Copy link
Member

Changed the issue to be a feature request since it doesn't really break anything, it has worked like that since the beginning.

Issues tagged with feature request are closed but tracked for reactions to gauge interest

@yacut
Copy link

yacut commented Aug 4, 2020

@jcesarmobile @dwieeb would you please explain that? I don't quite understand why it's not possible to configure a prefix for the storage key (remove prefix)? If capacitor is a good replacement for the old cordova project, then most likely cordova-plugin-nativestorage was used in the old project, which has no default prefix. With capacitor you should use the core storage plugin, which has _cap_ default prefix. So if you need to migrate application from cordova to capacitor and data in local storage is important, then it is not possible with capacitor core plugin and you have to stay with cordova-plugin-nativestorage.

NativeStorage.setItem('key1', 'value1'); // key1=value1

const res = await Plugins.Storage.get({ key: 'key1' });
console.log(res.value); // <== undefined

@agroves333
Copy link

agroves333 commented Aug 13, 2020

This is indeed an unexpected behavior that I thought for sure would be configurable. I would be willing to create a PR for the feature if necessary.

My current workaround is to just wrap the Capacitor Storage with a util class that removes the prefix from the get method. Also, support auto serialization like the old Cordova/Ionic Storage.

// app/utils/Storage.ts
import { Plugins } from '@capacitor/core';
const { Storage: CapStorage } = Plugins;

export default class Storage {
  static get(key): Promise<any> {
    return CapStorage.get({ key: key.replace('_cap_', '') }).then(item => {
      const value = item.value;
      return typeof value === 'object' ? JSON.parse(value) : value;
    });
  }

  static set(key, value): Promise<any> {
    const setValue = typeof value === 'object' ? JSON.stringify(value) : value;
    return CapStorage.set({ key, value: setValue });
  }

  static remove(key): Promise<any> {
    return CapStorage.remove({ key });
  }
}

Usage:

import Storage from 'app/utils';

NativeStorage.setItem('somekey', 'somevalue');
// or
Storage.set('somekey', 'somevalue');
// would both be retrievable by
Storage.get('somekey');

But this just seems ridiculous to have to do.

@imhoffd imhoffd reopened this Aug 14, 2020
@imhoffd imhoffd transferred this issue from ionic-team/capacitor Aug 14, 2020
@imhoffd imhoffd changed the title feat: allow to configure KEY_PREFIX of web implementation of 'Storage' feat: allow to configure KEY_PREFIX Aug 14, 2020
@imhoffd imhoffd added enhancement New feature or request plugin: preferences and removed triage labels Aug 14, 2020
@imhoffd imhoffd mentioned this issue Aug 14, 2020
4 tasks
@imhoffd imhoffd linked a pull request Aug 14, 2020 that will close this issue
4 tasks
@imhoffd
Copy link
Contributor

imhoffd commented Aug 14, 2020

@yacut For example on iOS, NativeStorage uses a custom UserDefaults group called "NativeStorage", and we use the standard UserDefaults at the moment (with a key prefix). To me, it doesn't make sense to share their group, but I could be convinced otherwise. Any changes to this behavior would result in a breaking change for apps unless we add in migration logic. I would like to exclude the ability to extract values from NativeStorage from the scope of this issue and instead focus on the original request: the ability to change the key prefix. This is a completely reasonable request and will be addressed in Capacitor 3. If you want, you can create a separate feature request for extracting values from the NativeStorage plugin.

@imhoffd
Copy link
Contributor

imhoffd commented Aug 14, 2020

And I just found out the Android implementation doesn't even use the prefix to begin with, so this will be very difficult to implement in a cross-platform way. We may need to provide a way to migrate data.

@imhoffd
Copy link
Contributor

imhoffd commented Aug 14, 2020

I am going to rewrite the iOS implementation to use a non-shared UserDefaults and rewrite the web implementation to use IndexedDB. This will remove the need for a prefix, and instead promote the idea of a "storage group". There will be a way to migrate data from the Capacitor 2 implementation.

@imhoffd
Copy link
Contributor

imhoffd commented Aug 14, 2020

Okay, the latest for this is: If you use the storage group "NativeStorage", it will be backwards-compatible with cordova-plugin-nativestorage. For web this means you can access the root level of localStorage, fulfilling the original feature request in this issue. We'd like devs to migrate their data from NativeStorage, because the clear() of the NativeStorage plugin on web seems to be very destructive (but please correct me if I'm mistaken).

On iOS and web, the Capacitor Storage plugin uses the storage group (default of "CapacitorStorage") as a prefix to separate key/values from other data. On Android, it uses the group as an entirely separate SharedPreferences file.

Please take a look at this PR and let me know your thoughts! I'll update this issue when dev releases are made for Capacitor 3 and the new storage plugin.

@yacut
Copy link

yacut commented Aug 19, 2020

@dwieeb Thank you, I can't wait for Capacitor 3 👍

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 30, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of the plugin, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants