Update typings for getItem to account for the null case

[Retsam]

The typings don't currently account for the null case when doing getItem: from typescript's perspective, doing getItem will always return an item, while in practice it might be null. This allows for unsafe code like:

localforage.getItem<{foo: string}>("non-existent-key")
    .then(item => item.foo); // Compiles okay, but runtime errors, since `item` is null.

This updates the typings so that the return value reflects the null case. The new usage looks like:

localforage.getItem<{foo: string}>(key)
    .then(item => {
         if (!item) { /* Handle non-existence: default value, or throw error, whatever */}
         return item.foo;
    })

Or the TS ! operator can be used to say "trust me, it won't be null":

localforage.getItem<{foo: string}>('non-existent-key')
     // Same error as original case... but this time it's the consumer's fault for using the unsafe `!` operator.
     .then(item => item!.foo);

---

I also added a note about #42 . Since it's a JSDoc comment, it'll show up in intellisense:

Technically, the typings could be written so that this function never returns undefined; however that depends on TS2.8. Anecdotally, that's probably fine, I think most TS users keep pretty up-to-date, but it seemed like a trivial reason to introduce a minimum version.

[thgreasi]

LGTM but this sounds like a breaking change to me, since preexisting code will start complaining about the new unhandled null return value.
I will retarget this against the v2.0 dev branch.
Thanks 👍

PS: As for the current version, I guess that consumers will have to do a localforage.getItem<MyObject | null>('key');

[thgreasi]

Could we please rebase over the v2.0-dev branch and retarget this PR against it? If so, we can merge this sooner than later.

[Retsam]

@thgreasi Sorry it took me so long to get to this; I've repointed the PR at the dev branch.

[Retsam]

FWIW, though I do think fixing typings can be considered a semver non-breaking change. At the least, I know several other packages seem to treat them that way, at least. To make an analogy, I've definitely had my linter find new lint errors after a minor version bump: yeah, it 'breaks' my compile step, but it doesn't break my runtime code.

Personally, as a consumer of types, I'd rather get notified of possible runtime errors sooner, so I'd probably put it as a minor version (not a patch), but if you want it in the v2 branch, that's fine too.

[thgreasi]

@Retsam you are right, I just faced this typing issue while writting a new custom driver in TS. Let's get this in v1.8, so that custom driver plugins are able to have a clear migration step.

[thgreasi]

Please change the base of your branch to be the current master, or if you prefer it I can try cherrypicking the commit, but in that case I'm not sure whether GH will consider the PR merged or not.

[Etheryte]

LGTM but this sounds like a breaking change to me, since preexisting code will start complaining about the new unhandled null return value.

A better way to look at this would be that preexisting code will now correctly warn about unhandled cases at build time instead of giving a runtime error. Having your build break when you update dependencies is a much better option than having your code break at runtime.

As a consumer, it would be nice to get this merged asap and be assured all the types are correct everywhere. Currently we need to <T | null> every getter and be doubly careful for human error.

[tofumatt]

I think @Etheryte's point makes sense; this is sort of a fix more than a breaking change. It will result in people having to update their code, but it will fail as expected, so it's really more like a bugfix. 🤷‍♂

[thgreasi]

Resolved in v1.9.0 by #980