-
Notifications
You must be signed in to change notification settings - Fork 135
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
Update LocalStorage error handling #666
Changes from all commits
2fb1a87
2f310a8
4e24777
e83c2cf
d2e410d
971318b
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@segment/analytics-next': patch | ||
--- | ||
|
||
Do not throw errors if localStorage becomes unavailable |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,16 +11,17 @@ function clear(): void { | |
localStorage.clear() | ||
} | ||
|
||
let store: LocalStorage | ||
beforeEach(function () { | ||
store = new LocalStorage() | ||
clear() | ||
}) | ||
|
||
describe('user', () => { | ||
const cookieKey = User.defaults.cookie.key | ||
const localStorageKey = User.defaults.localStorage.key | ||
const store = new LocalStorage() | ||
|
||
describe('()', () => { | ||
beforeEach(() => { | ||
clear() | ||
}) | ||
|
||
it('should pick the old "_sio" anonymousId', () => { | ||
jar.set('_sio', 'anonymous-id----user-id') | ||
const user = new User() | ||
|
@@ -61,7 +62,6 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
describe('when cookies are disabled', () => { | ||
|
@@ -294,15 +294,13 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
describe('when cookies are disabled', () => { | ||
beforeEach(() => { | ||
jest.spyOn(Cookie, 'available').mockReturnValueOnce(false) | ||
|
||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should get an id from the store', () => { | ||
|
@@ -332,7 +330,6 @@ describe('user', () => { | |
jest.spyOn(Cookie, 'available').mockReturnValueOnce(false) | ||
|
||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should get an id from memory', () => { | ||
|
@@ -444,7 +441,6 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should get traits', () => { | ||
|
@@ -537,7 +533,6 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should save an id to a cookie', () => { | ||
|
@@ -604,7 +599,6 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should reset an id and traits', () => { | ||
|
@@ -647,7 +641,6 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should save an id', () => { | ||
|
@@ -704,7 +697,6 @@ describe('user', () => { | |
|
||
beforeEach(() => { | ||
user = new User() | ||
clear() | ||
}) | ||
|
||
it('should load an empty user', () => { | ||
|
@@ -751,12 +743,6 @@ describe('user', () => { | |
}) | ||
|
||
describe('group', () => { | ||
const store = new LocalStorage() | ||
|
||
beforeEach(() => { | ||
clear() | ||
}) | ||
|
||
it('should not reset id and traits', () => { | ||
let group = new Group() | ||
group.id('gid') | ||
|
@@ -865,19 +851,36 @@ describe('group', () => { | |
}) | ||
|
||
describe('store', function () { | ||
const store = new LocalStorage() | ||
beforeEach(function () { | ||
clear() | ||
}) | ||
|
||
describe('#get', function () { | ||
it('should not not get an empty record', function () { | ||
expect(store.get('abc') === undefined) | ||
it('should return null if localStorage throws an error (or does not exist)', function () { | ||
const getItemSpy = jest | ||
.spyOn(global.Storage.prototype, 'getItem') | ||
.mockImplementationOnce(() => { | ||
throw new Error('getItem fail.') | ||
}) | ||
store.set('foo', 'some value') | ||
expect(store.get('foo')).toBeNull() | ||
expect(getItemSpy).toBeCalledTimes(1) | ||
}) | ||
|
||
it('should not get an empty record', function () { | ||
expect(store.get('abc')).toBe(null) | ||
}) | ||
|
||
it('should get an existing record', function () { | ||
store.set('x', { a: 'b' }) | ||
store.set('a', 'hello world') | ||
store.set('b', '') | ||
store.set('c', false) | ||
store.set('d', null) | ||
store.set('e', undefined) | ||
|
||
expect(store.get('x')).toStrictEqual({ a: 'b' }) | ||
expect(store.get('a')).toBe('hello world') | ||
expect(store.get('b')).toBe('') | ||
expect(store.get('c')).toBe(false) | ||
expect(store.get('d')).toBe(null) | ||
expect(store.get('e')).toBe('undefined') | ||
}) | ||
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 we can add some tests around .get() if the following values are stored:
...also a test if localStorage.getItem throws an error. 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 was able to cover all the values but having some trouble with making localStorage.getItem unavailable.
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. Use jest spies to mock global methods. it('should return null if localStorage throws', () => {
const getItemSpy = jest
.spyOn(global.Storage.prototype, 'getItem')
.mockImplementationOnce(() => {
throw new Error('getItem fail.')
})
store.set('foo', 'some value')
expect(store.get('foo')).toBeNull()
expect(getItemSpy).toBeCalledTimes(1)
}) |
||
}) | ||
|
||
|
@@ -900,16 +903,12 @@ describe('store', function () { | |
store.set('x', { a: 'b' }) | ||
expect(store.get('x')).toStrictEqual({ a: 'b' }) | ||
store.remove('x') | ||
expect(store.get('x') === undefined) | ||
expect(store.get('x')).toBe(null) | ||
}) | ||
}) | ||
}) | ||
|
||
describe('Custom cookie params', () => { | ||
beforeEach(() => { | ||
clear() | ||
}) | ||
|
||
it('allows for overriding keys', () => { | ||
const customUser = new User( | ||
{}, | ||
|
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.
Interesting, I wonder how this test was passing before when we should have been returning a value or null.
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.
expect(store.get('abc') === undefined)
lacks an actual assertion at the end, so it always passes. It's basicallyexpect(false)
orexpect(true)
haha --there's probably a lint rule for it =)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.
Ah of course, that makes sense.