Make verifyItem optional

[krawaller]

Two fixes to localStorageAtom:

• Make verifyItem optional by having a dummy default that always returns true
• Don't call verifyItem when there's no stored value, just used default

[merisbahti]

hmm ok so it's atleast checking prescence? what happens if the stored value is a boolean false?

[krawaller]

It isn't checking presence, it'll always return true. I just included the param in the expression to make TS shut up about unused params. :)

[merisbahti]

after actually reading the code I got it, thanks. LGTM!

[krawaller]

PR updated with fix for not using verifyItem when no stored value, as discussed in #17

[merisbahti]

  const initialItem = (() => {
    if (storedItem === null) {
      return initialValue
    }
    if (verifyItem(parsedItem)) {
      return parsedItem
    }
    return initialValue
  })()

Here's basically how I visualize it being done, it doesn't matter if the parsing fails/succeeds if the storedItem is not existent, hmm, now, to find a case where this "fixes" something.

[merisbahti]

Ok took some time, but here's a case:

it('correctly allows null to be stored', async () => {
  localStorage.removeItem('myOtherKey')
  localStorage.setItem('myOtherKey', JSON.stringify(null))
  const atom = localStorageAtom(
    'foo',
    'myOtherKey',
    (v): v is null | 'foo' => v === null || v === 'foo',
  )
  expect(atom.getValue()).toBe(null)
})

[krawaller]

Ah! Good catch, good test!

[krawaller]

Updated according to Bahtijaragic wisdom above, + handling of unparseable stored values.

[krawaller]

Oh! This new failing test is actually interesting. Some thinking required around how you want that to behave!

Just me being dumb! Sorry! 🙈

[merisbahti]

Oh! This new failing test is actually interesting. Some thinking required around how you want that to behave!

Exactly...

I want to live in a world where we actually verify things at IO boundaries (at runtime), and don't just "assert" that stuff is a type at compile time. I think it's one of the most broken things about other peoples perception of typescript.

But, that's not the world we live in. So we need to allow "bad typescript habits".

In this case, if there's no verifier - we should without verificiation return what's stored in localstore.

[merisbahti]

So just to be clear:

This is the behavior I'm suggesting. It's a 100% footgun, and it's a dangerous API. I think the most problematic thing about this is, that it's a dangerous-foot-gun-api that actually looks pretty aesthetically pleasing.

const myAtom = localStorageAtom('defaultValue', 'mykey')

Loooks way more pleasing than

const myAtom = localStorageAtom('defaultValue', 'mykey', value => typeof value === 'string')

But is way more dangerous, since the aesthetically pleasing one is falsely coercing typescript about that the type of myAtom is string.

In my dreamworld, that kind of behavior should be avoided by making a really ugly api, like this (example)

const myAtom = dangerous$__$LocalStorageAtom({__defaultValue: 'defaultValue'}, {theKeyIs: 'mykey'})

Or similar :P

[krawaller]

So, I was actually just being dumb, in my brain !verifier || verifier(val) is what I meant to write. And then I misread the fail. Apologies! :D

[krawaller]

Honestly, even if the API enforced passing in a verifier, people like me will still just pass in some dumb always true crap. 😁

[merisbahti]

Yes, I agree, all things considered this is 100% the way to go!

[krawaller]

So, what's the politics here? Am I to push the button myself? :D

[merisbahti]

[merisbahti]

wooohoo! this is unprecedented (for klyva)! first PR by an other soul

[krawaller]

🎉