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

Switch use of IndexedDB openCursor to openKeyCursor for fetching keys #874

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

MeMark2
Copy link

@MeMark2 MeMark2 commented Feb 8, 2019

First off, first time contributing to this project. Let me know if I'm doing anything wrong.

A little bit of background:
I'm running into a problem with some of my production IndexedDB databases which seems to be related to accessing the whole db with openCursor. I haven't been able to figure out why I'm having this problem or how to reproduce it with a db that doesn't contain sensitive information, but I did notice that using openKeyCursor for the key and keys calls (in the IndexedDB driver) lets me work around the problem for now.

Is there a particular reason we're using openCursor instead of openKeyCursor for the key and keys call in the IndexedDB driver? Is there compatibility issues I'm not aware of? As far as I know, using openKeyCursor in this case would be much faster and is just more appropriate since we don't need any actual values for these calls.

@lincolnthree
Copy link

@MeMark2 @tofumatt I've been testing this PR both locally, and now deployed to test, and yesterday to production. It seems this has settled some of the IDBExceptions that we've been seeing with Safari on iOS 13.

@f
Copy link

f commented Jun 1, 2020

Confirmed. This seems to be working.

@tofumatt
Copy link
Member

tofumatt commented Jun 4, 2020

I figured it could have been a compatibility issue, but the two APIs don't seem to differ in their platform support. So honestly: I'm not quite sure why that API was used to get keys. Might've just been that we originally (years ago) used it to get keys and never improved on it. A lot of localForage was written over five years ago, and I barely remember what I ate for lunch yesterday! 😅

I'm reviewing this PR now (I was away last week), thanks for your patience! 🙂

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.

This seems straightforward, passes tests, should improve performance, and fixes bugs. Yay! 🎉

I'm going to rebuild the dist files and push to this branch, get this merged into master, and then push a release. Thank you very much for the PR! 🙏

@tofumatt tofumatt merged commit 566f9e8 into localForage:master Jun 4, 2020
@tofumatt
Copy link
Member

tofumatt commented Jun 5, 2020

Just an update that this has been published on npm as is available in 1.7.4. Thanks again for the PR 😄

@zhouxiaoping
Copy link

zhouxiaoping commented Oct 28, 2020

Which versions of Edge does this happen with? Could you file an issue for this so we can address it specifically?

userAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36 Edge/18.18363

merge request for Edge18 compatibility:#971

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

6 participants