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

IndexedDBShim makes Chrome and Firefox use WebSQL #263

Closed
bsautel opened this issue Oct 17, 2016 · 4 comments
Closed

IndexedDBShim makes Chrome and Firefox use WebSQL #263

bsautel opened this issue Oct 17, 2016 · 4 comments
Assignees
Milestone

Comments

@bsautel
Copy link

bsautel commented Oct 17, 2016

First, thanks for writing this polyfill that is one of the rare ways that exist to use a decent database in a web browser through a unified API.

I use Dexie.js (an IndexedDB wrapper) and I noticed a strange behavior when using it with IndexedDBShim.

I would expect Dexie.js and IndexedDBShim to use the most accurate underlying database according to the browser and it does not seem to be the case.

I noticed that my code uses always WebSQL, even in Chrome and Firefox that correctly support IndexedDB yet according to Can I use, and it does not work at all on Firefox since it does not support WebSQL. I reported that issue in the Dexie.js project: dexie/Dexie.js#342.

This issue enabled me to understand why it does not work in Firefox, Dexie.js uses IndexedDBShim first whereas IndexedDBShim recommends to use IndexedDB first. It sounds like they use this behavior because of to an issue with Safari 9.

The documentation says that IndexedDBShim does nothing on browsers that support IndexedDB. But it appears that in Firefox it defines the window.shimIndexedDB variable anyway and it contains some code using WebSQL that has no chance to work. When the documentation says that it does nothing, does it mean it does not modify the window.indexedDB variable? Is there a reason why the window.shimIndexedDB is defined in the case there is nothing to do such as in Firefox?

I also noticed that if I call window.shimIndexedDB.__useShim() to make the shim fix issues in browsers with broken or partial implementation (what I need), Chrome uses WebSQL whereas it seems to support IndexedDB. It sounds like it would be better to use IndexedDB in the cases it is correctly supported, wouldn't it?

Is there a way to use IndexedDB if its implementation in the current browser is satisfying and the WebSQL binding in other cases?

If it is not the case, I still can detect it manually and use window.indexedDB or window.shimIndexedDB according to the browser capacities but who better than IndexedDBShim knows that?

Thanks in advance.

@bsautel bsautel changed the title The shim makes Chrome use WebSQL IndexedDBShim makes Chrome use WebSQL Oct 17, 2016
@bsautel bsautel changed the title IndexedDBShim makes Chrome use WebSQL IndexedDBShim makes Chrome and Firefox to use WebSQL Oct 17, 2016
@bsautel bsautel changed the title IndexedDBShim makes Chrome and Firefox to use WebSQL IndexedDBShim makes Chrome and Firefox use WebSQL Oct 18, 2016
@brettz9 brettz9 added this to the 3.0.0 milestone Jan 20, 2017
@brettz9 brettz9 self-assigned this Jan 20, 2017
@brettz9 brettz9 added this to Chrome/Firefox in Browser bugs Feb 16, 2017
@nagyf
Copy link

nagyf commented Mar 31, 2017

Any updates on this? This pretty much makes this lib unusable on chrome. I don't want my webapp to use WebSQL... Are there any workarounds?

@brettz9
Copy link
Collaborator

brettz9 commented Mar 31, 2017

For info on the requirements before the next release, see #263 . Help is welcome to get things up to speed. Specific browser support is the last item on my list for the release, so feel free to investigate whether the issue still exists with the version in master. The responsible code is in src/setGlobalVars.js.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 16, 2017

Finally getting to take a look at browser-specific issues...

I would expect Dexie.js and IndexedDBShim to use the most accurate underlying database according to the browser and it does not seem to be the case.

While our code tries to do this for you (more on that below), I should say it is not always completely easy to determine what this is. There may be features now fixed in our polyfill which fail in the native version and there are known cases where it is the other way around even in our best polyfilling environment (currently Node.js followed by Chrome with WebSQL).

I noticed that my code uses always WebSQL, even in Chrome and Firefox that correctly support IndexedDB yet according to Can I use, and it does not work at all on Firefox since it does not support WebSQL. I reported that issue in the Dexie.js project: dexie/Dexie.js#342.

Our code shouldn't be attempting to replace the native version automatically except in iOS9 and non-Chrome Android which were determined to be poorly supported. Please subscribe to issue #277, however, regarding these two cases, as I plan to post there shortly.

If Dexie is assuming the presence of shimIndexedDB indicates it should be used, this is indeed a problem as that is probably only a useful assumption with some (all?) webkit versions which would not allow redefining indexedDB. It would be better for it to just use indexedDB and follow the instructions here on how to define it if not present: https://github.com/axemclion/IndexedDBShim#ios

As far as Can I Use, note that this is oversimplifying a bit--there are well over 1000 tests from the W3C and other sources that we are using to gauge compliance and native implementations are not passing all of these tests (we are not either, but coming close to what is feasible for us to implement, especially in Node; in the browser, specific browsers have their own remaining issues which we need help debugging).

This issue enabled me to understand why it does not work in Firefox, Dexie.js uses IndexedDBShim first whereas IndexedDBShim recommends to use IndexedDB first. It sounds like they use this behavior because of to an issue with Safari 9.

Yes, but see above on what approach I think they should be taking.

Btw, for Safari 9, our code automatically attempts to add all interfaces.

The documentation says that IndexedDBShim does nothing on browsers that support IndexedDB. But it appears that in Firefox it defines the window.shimIndexedDB variable anyway and it contains some code using WebSQL that has no chance to work. When the documentation says that it does nothing, does it mean it does not modify the window.indexedDB variable?

The WebSQL code is not run unless you attempt to run it. However, I agree we could probably instead avoid by providing some otherwise empty "shimIndexedDB" object which only had the __useShim method that would alone cause shimIndexedDB itself to become an IDBFactory object. PRs welcome, but I don't think this ought to be a big problem if the library is used correctly.

Is there a reason why the window.shimIndexedDB is defined in the case there is nothing to do such as in Firefox?

See the above paragraph. We could also potentially be patching implementations without any need for WebSQL, btw, though not sure there is any need in Firefox.

Note that the other interfaces are not added automatically except for iOS9 and non-Chrome Android or if indexedDB could not be overwritten and there is an openDatabase implementation, or if there is a webkit*-prefixed implementation of the interface (not sure we're getting all of them if they're still around though).

But again, I agree it doesn't make sense for the IDBFactory interface to be exposed to Firefox via shimIndexedDB even while it does not automatically attempt to overwrite indexedDB except in the specific browsers with poor support just mentioned.

I also noticed that if I call window.shimIndexedDB.__useShim() to make the shim fix issues in browsers with broken or partial implementation (what I need), Chrome uses WebSQL whereas it seems to support IndexedDB. It sounds like it would be better to use IndexedDB in the cases it is correctly supported, wouldn't it?

Probably yes, as Chrome's will certainly be faster, and there are a few issues mentioned in the README that our polyfill, as a non-native app, cannot address. There may be some cases, however, where we are more compliant with the spec than with Chrome (and I would like to see about improving our Chrome support as it has a few more failing tests (documented in node-good-bad-files.js) than the Node version for whatever reason), but as the browser manufacturers create the spec, often adjusting it according to actual implementation status, this is even more reason to follow browser behavior, especially one such as Chrome which is far along in the game. But if for nothing else than testing, we do provide the option to turn it on, and it may patch some temporary issues faced by the native implementation.

Is there a way to use IndexedDB if its implementation in the current browser is satisfying and the WebSQL binding in other cases?

That's how it should be working, at least in master. Let me know what browser you are using if you are having such problems with the version in master.

If it is not the case, I still can detect it manually and use window.indexedDB or window.shimIndexedDB according to the browser capacities but who better than IndexedDBShim knows that?

There is not usually much need to use anything besides indexedDB which will be auto-polyfilled where support is poor and otherwise you will be left to turn it on with shimIndexedDB.__useShim(). shimIndexedDB is mostly useful for its methods of defining configuration. However, as per the README, webkit has had (still has?) an issue allowing indexedDB to be overwritten, so shimIndexedDB can be used in that case, or a local indexedDB variable (as in a closure) defined.

HTH...

In sum, further action is warranted in regard to this issue as follows: Avoid adding/exposing IDBFactory methods on shimIndexedDB where WebSQL is not supported.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 22, 2017

I've added #299 to address the concern about avoiding adding/exposing IDBFactory methods on shimIndexedDB where WebSQL is not supported, but especially with Dexie's own fix now, the original reported issue here can I think be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Browser bugs
Chrome/Firefox
Development

No branches or pull requests

3 participants