Update LocalStorage error handling

[ryder-wendt]

This PR is created to fix #631

LocalStorage was reported as becoming unavailable during an IOS private safari session. While unable to reproduce, try/catch have been added enclosing the line where the error was thrown with the intention to catch the error before surfacing on downstream tools.

Testing

---

[x] Minor unit test added

[changeset-bot]

🦋 Changeset detected

Latest commit: 971318b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-next	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[chrisradek]

I think we should match the warnings we have for set and get here as well.

[silesky]

^ to follow up, we may end up wanting to create a logStorageWarning function so we don't repeat ourselves and avoid duplicate strings -- they take up precious bytes.

[chrisradek]

Interesting, I wonder how this test was passing before when we should have been returning a value or null.

[silesky]

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 =)

[chrisradek]

Ah of course, that makes sense.

[silesky]

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.

[ryder-wendt]

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

[silesky]

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

[silesky]

"should return null if localStorage throws an error (or does not exist)" would be an improved description

[ryder-wendt]

Agreed!

[silesky]

@ryder-wendt this PR should have a changeset (patch version bump of analytics-next), allowing us to autorelease this library when we choose and giving you credit for it. See: https://github.com/segmentio/analytics-next/blob/master/RELEASING.md#what-is-a-changeset

[silesky]

A changeset can be as long as you want -- it should usually be a bit nicer than a commit, as it will be used to generate changelog. I would phrase this as "Do not throw errors if localStorage becomes unavailable".

[ryder-wendt]

This makes sense, I was using @1.46.0 as a reference but now looking at all changeset's, they do appear to be more descriptive.