-
Notifications
You must be signed in to change notification settings - Fork 83
Add support for storage options #68
base: master
Are you sure you want to change the base?
Add support for storage options #68
Conversation
d3c7098 to
0fa74ff
Compare
|
@emeraldsanto could you have a look at this? We are storing JWT refresh tokens in It would make a lot of sense for us to set #91 also points a potential issue with this. |
|
This would be helpful to have for our business use case as well 👍 |
| @@ -1 +1,3 @@ | |||
| export { default } from './EncryptedStorage'; | |||
| export { EncryptedStorageOptions } from './EncryptedStorage'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be export type, otherwise it will fail when using a bundle transformer stricter than the native Babel one (in my case, esbuild).
|
|
||
| private String getStorageName(ReadableMap options) { | ||
| String bundleId = this.getReactApplicationContext().getPackageName(); | ||
| String storageName = options.hasKey("storageName") ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not setting any default value for options, nor checking if is it defined.
This line throws if called without an options, for example:
EncryptedStorage.getItem("myValue")
|
@emeraldsanto any plans to merge this PR? This'll solve several issues all of us are facing. |
|
Will this pull request be merged? |
|
Also looking for this to be merged as it would resolve thousands of Sentry errors. Any update? |
|
Any updates? @emeraldsanto |
|
I have same problem. Should I wait for merge or make patch? |
Hi @emeraldsanto, thank you for this great library! This PR adds support for storage options, namely:
keychainAccessibilityon iOS, via kSecAttrAccessiblestorageName, which uses kSecAttrService on iOS and fileName on AndroidBoth are optional and have sensible defaults (which we can change if you think it's required).
In terms of the API, I added an
optionsobject as the previous to last parameter to all methods (before the optionalcb).Following semver, this would be a breaking change in the function signature, so I suppose it requires a major version bump (if you agree with the API / approach).
Please let me know what you think.