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

Update LocalStorage error handling #666

Merged
merged 6 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/itchy-points-flash.md
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
63 changes: 31 additions & 32 deletions packages/browser/src/core/user/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -61,7 +62,6 @@ describe('user', () => {

beforeEach(() => {
user = new User()
clear()
})

describe('when cookies are disabled', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -332,7 +330,6 @@ describe('user', () => {
jest.spyOn(Cookie, 'available').mockReturnValueOnce(false)

user = new User()
clear()
})

it('should get an id from memory', () => {
Expand Down Expand Up @@ -444,7 +441,6 @@ describe('user', () => {

beforeEach(() => {
user = new User()
clear()
})

it('should get traits', () => {
Expand Down Expand Up @@ -537,7 +533,6 @@ describe('user', () => {

beforeEach(() => {
user = new User()
clear()
})

it('should save an id to a cookie', () => {
Expand Down Expand Up @@ -604,7 +599,6 @@ describe('user', () => {

beforeEach(() => {
user = new User()
clear()
})

it('should reset an id and traits', () => {
Expand Down Expand Up @@ -647,7 +641,6 @@ describe('user', () => {

beforeEach(() => {
user = new User()
clear()
})

it('should save an id', () => {
Expand Down Expand Up @@ -704,7 +697,6 @@ describe('user', () => {

beforeEach(() => {
user = new User()
clear()
})

it('should load an empty user', () => {
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor

@silesky silesky Nov 9, 2022

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 basically expect(false) or expect(true) haha --there's probably a lint rule for it =)

Copy link
Contributor

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.

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')
})
Copy link
Contributor

@silesky silesky Nov 10, 2022

Choose a reason for hiding this comment

The 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:

  • "hello world"
  • ""
  • undefined -- should just pass through "undefined" AFAIK instead of any coercion, so people can easily debug, since it should be an impossible state. Or we could coerce to null. I could go either way.
  • false
  • null

...also a test if localStorage.getItem throws an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
I'm only able to throw by messing with Storage.prototype but it has to be placed at bottom of test file, doesn't feel like best practice.

  const store = new LocalStorage()
  beforeEach(function () {
    clear()
  })

  it('should catch without localStorage.getItem', function () {
    store.set('x', 'test')
    Storage.prototype.getItem = null as any
    expect(store.get('x')).toBe(null)
  })
})

Copy link
Contributor

@silesky silesky Nov 10, 2022

Choose a reason for hiding this comment

The 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)
    })

})

Expand All @@ -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(
{},
Expand Down
25 changes: 19 additions & 6 deletions packages/browser/src/core/user/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ class NullStorage extends Store {
remove = (_key: string): void => {}
}

const localStorageWarning = (key: string, state: 'full' | 'unavailable') => {
console.warn(`Unable to access ${key}, localStorage may be ${state}`)
}

export class LocalStorage extends Store {
static available(): boolean {
const test = 'test'
Expand All @@ -149,29 +153,38 @@ export class LocalStorage extends Store {
}

get<T>(key: string): T | null {
const val = localStorage.getItem(key)
if (val) {
try {
const val = localStorage.getItem(key)
if (val === null) {
return null
}
try {
return JSON.parse(val)
} catch (e) {
return JSON.parse(JSON.stringify(val))
return val as any as T
}
} catch (err) {
localStorageWarning(key, 'unavailable')
return null
}
return null
}

set<T>(key: string, value: T): T | null {
try {
localStorage.setItem(key, JSON.stringify(value))
} catch {
console.warn(`Unable to set ${key} in localStorage, storage may be full.`)
localStorageWarning(key, 'full')
}

return value
}

remove(key: string): void {
return localStorage.removeItem(key)
try {
return localStorage.removeItem(key)
} catch (err) {
localStorageWarning(key, 'unavailable')
}
}
}

Expand Down