-
-
Notifications
You must be signed in to change notification settings - Fork 82
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 window or global to allow SSR compatibility #51
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import root from 'window-or-global'; | ||
import utils from './utils'; | ||
import constants from './constants'; | ||
import enc from './enc-utf8'; | ||
|
@@ -9,6 +10,13 @@ import DES from 'crypto-js/tripledes'; | |
import RABBIT from 'crypto-js/rabbit'; | ||
import RC4 from 'crypto-js/rc4'; | ||
|
||
const globalStorage = { | ||
setItem: () => {}, | ||
getItem: () => {}, | ||
removeItem: () => {}, | ||
clear: () => {} | ||
}; | ||
|
||
export default class SecureLS { | ||
constructor(config) { | ||
config = config || {}; | ||
|
@@ -36,7 +44,7 @@ export default class SecureLS { | |
config.encodingType.toLowerCase() : | ||
constants.EncrytionTypes.BASE64; | ||
|
||
this.ls = localStorage; | ||
this.ls = root.localStorage || globalStorage; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please also set the storage type on window?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not aware of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I may not provided enough context to the PR, sorry. I've had an issue with SSR in react this is my current work around when using SecureLS: const getSecureLS = () => {
if (typeof window !== 'undefined') {
return new SecureLS({});
}
return {
set: () => ({}),
get: () => ({}),
remove: () => ({}),
};
}; |
||
this.init(); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import sinon from 'sinon'; | ||
import SecureLS from '../dist/secure-ls.js'; | ||
|
||
describe('root - LocalStorage Tests->', () => { | ||
let lib; | ||
|
||
beforeEach(() => { | ||
global.localStorage = { | ||
setItem: sinon.spy(), | ||
getItem: sinon.spy(), | ||
removeItem: sinon.spy(), | ||
clear: sinon.spy() | ||
}; | ||
lib = new SecureLS(); | ||
}); | ||
|
||
describe('setItem method', () => { | ||
it('set should call setItem on localStorage', () => { | ||
const data = [1, 2, 3]; | ||
const key = 'key-1'; | ||
|
||
lib.set(key, data); | ||
sinon.assert.calledWith(global.localStorage.setItem, key); | ||
}); | ||
}); | ||
|
||
describe('getItem method', () => { | ||
it('get should call getItem on localStorage', () => { | ||
const key = 'key-1'; | ||
|
||
lib.get(key); | ||
sinon.assert.calledWith(global.localStorage.getItem, key); | ||
}); | ||
}); | ||
|
||
describe('removeItem method', () => { | ||
it('remove should call removeItem on localStorage', () => { | ||
const key = 'key-1'; | ||
|
||
lib.remove(key); | ||
sinon.assert.calledWith(global.localStorage.removeItem, key); | ||
}); | ||
}); | ||
|
||
describe('clear method', () => { | ||
it('clear should call clear on localStorage', () => { | ||
lib.clear(); | ||
sinon.assert.calledWith(global.localStorage.clear); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3739,6 +3739,11 @@ wide-align@^1.1.0: | |
dependencies: | ||
string-width "^1.0.2 || 2" | ||
|
||
window-or-global@^1.0.1: | ||
version "1.0.1" | ||
resolved "https://registry.yarnpkg.com/window-or-global/-/window-or-global-1.0.1.tgz#dbe45ba2a291aabc56d62cf66c45b7fa322946de" | ||
integrity sha1-2+RboqKRqrxW1iz2bEW3+jIpRt4= | ||
|
||
[email protected]: | ||
version "0.1.0" | ||
resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.1.0.tgz#5438cd2ea93b202efa3a19fe8887aee7c94f9c9d" | ||
|
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.
Shouldn't it be:
this will allow overriding the global object and defining your own. Thoughts?
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.
What would
customStorage
be?Are you assuming this would be on the global object when used in a node environment? If so maybe it could be set as a property for use in server node apps. I'm not sure this library would be used on the server.
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 would help in defining custom methods(get, set, etc.) when localStorage is not present(in case of incognito mode and cookies disabled scenario). It will help there.
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.
localStorage is available in incognito. I'm not quite sure what scenario you are suggesting can you specify a browser etc.
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.
@softvar any final thoughts?