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 localforage.d.ts #980

Merged
merged 1 commit into from Jul 31, 2020
Merged

Update localforage.d.ts #980

merged 1 commit into from Jul 31, 2020

Conversation

Glandos
Copy link
Contributor

@Glandos Glandos commented Jul 24, 2020

getItem can return null when the item doesn't exist

getItem can return null when the item doesn't exist
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think our TypeScript definitions in general are lacking, through our 2.0 rewrite is actually in TypeScript which should help 😄

Changing this has the potential to break things though—it would read like a breaking change. I'm for doing that, but it's best-reserved for a major release, eg 2.0.

So while this makes sense I don't think we should be merging it and shipping it, as it could break existing builds 🤔

Thoughts?

@Glandos
Copy link
Contributor Author

Glandos commented Jul 27, 2020

It's a nice debatable question. I personally opened this because I started using localforage, and the bad Typescript signature leads me to a bug, with an uncatched null value.
In my opinion, this was a bug that should be resolved. If it breaks build, it means that projects had bugs that should be resolved too. However, it also requires a huge warning sign in the changelog 😉

But if the 2.0 rewrite is not far from complete (let's say, 6 months), then it doesn't really make sense to annoy people with that. This existing issue should guide people to a workaround in the meantime.

Copy link
Member

@thgreasi thgreasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with it since it makes the typings more accurate.
This was actually initially mentioned in #858
Even though this is essentially a patch, I'm fine releasing as a minor so that consumers using ~ in their package.json will not have any issues with their CI.
cc: @tofumatt

@tofumatt
Copy link
Member

Fair enough, let's ship this as 1.9.

I'll put it together tomorrow 😄

@tofumatt tofumatt merged commit b38ba45 into localForage:master Jul 31, 2020
@tofumatt
Copy link
Member

tofumatt commented Aug 1, 2020

Just an update: this was published as localForage 1.9 today. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants