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
localforage causing race conditions with IndexedDB and WebSQL #856
Comments
I found the root cause of the issue in the function that detects if isIndexDb is valid. The issue here is that browsers can be emulating behaviors with polyfills.
|
I'll gladly submit a PR for this, would like a little guidance though. |
Ah, interesting! Thanks for investigating this one 👍 We should try to mitigate this bug but it feels like more of a bug in LastPass. My gut feeling is they shouldn't be overriding that native method 🤷♂️. The whole point of our check for native code was that we wanted to know what the browser's native capability was so we could, effectively, UA sniff. I guess we're both doing bad things here 😆 LastPass is definitely big enough for this to be worth fixing though, and maybe even with a specialised patch? My first instinct is to just check for Safari's version number here instead of our If you could replace the If it's not something we can do reliably and without a lot of bloat, simply checking for LastPass (does it leave any markers of it running in the global scope?) and running the check differently could work. Maybe there's another Safari 10.1 feature check available? |
I guess the point is, you can fully polyfill both indexeddb and fetch. There are so many random storage mechanisms built into older browsers that an indexedDb polyfill becomes useful to take advantage of them. And the same goes for fetch :). I've already filed a bug with LastPass, but I could easily see this behavior happening in other situations. with polyfills. My only suggestion for this lib would be to do a different check (as you're suggesting) or simply drop the "native code" part of the qualification. |
Also as an addendum, if window.indexedDb is available, why do you care about fetch at all? |
If IndexedDB were polyfilled it would definitely cause errors with localForage. localForage assumes a native and pretty robust IndexedDB that is able to handle a lot of JS primitives that a polyfilled library would–likely–not support. Just guessing though. The context for why we do this is actually because some IndexedDB implementations (like Safari's until 10.1) aren't good enough for us to rely on, so we use WebSQL instead. I just realised that the comment that explains that has an issue reference for PouchDB, so I'll edit that comment. Here's the issue though: pouchdb/pouchdb#5572
But we do need the check because we can't have older, buggy Safari versions using IndexedDB. |
Got it. I’ll put in a PR with appropriate safari detection. |
this check also causes Errors in our project, which relies on |
is it fixed? |
We use ZoneJs in Angular project wich also polyfill fetch function, need better way to |
FWIW, this is causing intermittent issues for us as well - hasFetch is returning false when we're in Chrome 78. |
You're right. This is extremely problematic because there are lots of libraries that polyfill fetch for various reasons, e.g. Fullstory and/or Sentry polyfill it for instrumentation. |
Any update on this? We are also running in to these issues :/ |
Not yet, but I can definitely understand this is a bit too protective. I'd happily accept a PR for this functionality, but I won't have time to cook one up myself until at least later this week. If anyone can make a PR (preferably with some tests to cover polyfills), we should be able to ship this as a patch release. |
Am I correct in assuming that the following line:
Should be replaced with:
If so, happy to create a PR, though not really sure what adding test case would involve in this case. Suggestions? |
I think that's it; probably best to have a test that polyfills |
Not sure I 100% followed you there. Here's the commit I've got so far. Do you have any examples of how you'd want to polyfill in a test? Wouldn't that pollute the environment? (I've never polyfilled anything before so might be better if I left this to you.) |
Hmm, our test code is so manual and old it's not a great place to test polyfills. 😓 I think your commit is a good start. If it passes the tests then we already aren't testing for that particular behaviour and I think it's fine to ship. If you wanted to start a PR with that change I think it'd be good, and I can look at adding a test—like you say—if it's straightforward. Thanks! |
@tofumatt I know the feeling... At least you have tests ;) PR submitted & linked. |
@tofumatt Hey! I hope all is well. Any chance we could get this reviewed/merged? I know life is busy but... this is still an issue. Just hoping :) |
Edit, I do see you've been making updates! Thank you! |
@lincolnthree this issue is causing some hiccups for us as well 😅 any chance that PR is going to be merged soon? |
Just commenting that I've just encountered this issue with LastPass as well. |
Interesting bug that took me forever to track down.
Basically, unless you explicitly set driver in localforage.config, Chrome and Safari are indeterministic about which driver they will use, resulting in what "seems" like data loss. This happens at random.
So basically, I have 2 databases, and I'll be connected to one of them through localforage on any given reload.
UPDATE:
IndexedDB detection flat out fails half the time on chrome. I've tried debugging this for the past hour and haven't made any progress. Setting the driver explicitly to indexeddb leads to "storage not found" errors.
The text was updated successfully, but these errors were encountered: