10 add support for custom storage type - [NEEDS WORK] #11
Conversation
|
I actually wrote a test for the additional functionality, should I do something about the decreased test coverage? |
|
@softvar it would be great if you could take a look at this, thank you! |
|
@fonkgoku @4F2E4A2E Sure. kinda busy with office work, will check this weekend surely. |
|
@softvar I added some more tests, looking forward to your review :) |
src/index.js
Outdated
| @@ -35,7 +35,7 @@ export default class SecureLS { | |||
| constants.EncrytionTypes.BASE64; | |||
| this.config.encryptionSecret = config.encryptionSecret; | |||
|
|
|||
| this.ls = localStorage; | |||
| this.ls = config.storageType || localStorage; | |||
There was a problem hiding this comment.
This is wrong! It should be an object and not a string.
The code should be:
this.config.storageType = config.storageType || 'localStorage';
this.ls = config.storageType || localStorage;
if (config.storageType && config.storageType === 'sessionStorage') {
this.storage = sessionStorage;
} else {
this.storage = localStorage;
}
There was a problem hiding this comment.
I'm a bit confused, you said "It should be an object and not a string", but in your example this.config.storageType is set to a string and not an object 🤔 . Also setting a custom storage type (something like mockStorage) is not possible the way you proposed.
src/index.js
Outdated
| @@ -35,7 +35,7 @@ export default class SecureLS { | |||
| constants.EncrytionTypes.BASE64; | |||
| this.config.encryptionSecret = config.encryptionSecret; | |||
|
|
|||
| this.ls = localStorage; | |||
| this.ls = config.storageType || localStorage; | |||
There was a problem hiding this comment.
Also, please update this.ls with this.storage since ls denotes localStorage and since you have added the support for sessionStorage as well, we should update the naming. Doing this will break a lot of test cases, so please find and replace all the instances of lib.ls in tests folder and update it with lib.storage.
There was a problem hiding this comment.
I actually thought about doing that at first, but hesitated because of all the test cases. I will go ahead and change it.
| @@ -296,4 +296,17 @@ export default class SecureLS { | |||
| return this.get(this.utils.metaKey, true); | |||
| }; | |||
|
|
|||
| // enable compatiblity with the sessionStorage/localStorage api | |||
| getItem(key, isAllKeysData) { | |||
There was a problem hiding this comment.
These methods are not required because we need users to use our API so that we can get.set.remove from our own bucket of information. Using method: getAllKeys - will return all the keys so far set. But if we use the methods you added, we won't be able to log those keys. So, please remove them.
Let me know if you need more explanation here.
There was a problem hiding this comment.
I don't see how these additional methods inhibit getting the information "from our own bucket of information" in any way, they are just references to the original get, set and remove methods. getAllKeys works fine, I locally modified the test in standard.spec.jsL60 to make sure. You can see for yourself if you like 🙂.
Also having these functions is essential for my team's project requirements. This way you can use SecureLS as a drop in solution for sessionStorage/localStorage in all kinds of other frameworks and libraries.
For example, with an apollographql plugin (https://github.com/apollographql/apollo-cache-persist):
import { persistCache } from 'apollo-cache-persist';
import SecureLS from 'secure-ls';
const cache = new InMemoryCache({...});
persistCache({
cache,
storage: new SecureLS(),
});
...
There was a problem hiding this comment.
Also having these functions is essential for my team's project requirements. doesn't mean everyone wants it, right? In that particular case, you should fork the repo, do the changes and use it as per your requirement.
from our own bucket of information means that when this library sets/removes/gets anything, it knows what all was set using it. Consider it like a jar which has all the information.
There was a problem hiding this comment.
doesn't mean everyone wants it, right?
True, but I don't see any disadvantage in having them. As outlined in my previous comment, the requirements on which your arguments are grounded in order to dismiss my changes are actually being fulfilled. I don't see how I can convince you any further 😬. If we don't find an agreement here, I will in fact take your advice, just fork the repo and change it to my requirements.
|
I have reviewed the PR and left some comments. Addressing those comments would ensure the library would be working fine with both the types. Also, since we have examples too, merging this PR without having sessionStorage examples would be incomplete. Please take out some time to add some sessionStorage examples in different files. Feel free to ask if you have any doubt. Thanks! |
softvar
changed the title
|
Any updates @fonkgoku ? Please let me know if you need any help from my side. |
|
Sorry for the late reply, I was still waiting for a reply on this comment #11 (comment) |
Closes #10