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

Support shimIndexedDB #65

Merged
merged 2 commits into from
Sep 1, 2015
Merged

Support shimIndexedDB #65

merged 2 commits into from
Sep 1, 2015

Conversation

rubenstolk
Copy link
Contributor

Important because for example safari doesn't allow overwriting this (see https://github.com/axemclion/IndexedDBShim), useful for cordova apps.

@jensarps
Copy link
Owner

jensarps commented Sep 1, 2015

Thanks for the PR!

Well, that is a delicate issue.

IDBWrapper already checks for shimIndexedDB, but it will prefer indexedDB if available. For older Safari's without IndexedDB support that means it will take the shim to work with, and all will be good™. For Safari 8 however, it will use Safari's own buggy IDB implementation.

One could, however, prefer shimIndexedDB over indexedDB, but I'd rather not do this; a shim should always be the last resort. And who knows, maybe Apple gets IndexedDB right in some future Safari release.

@rubenstolk
Copy link
Contributor Author

I don't fully agree, if the shim is not needed, the shim should not be exposed. (A good shim implementation would anyways always fallback to a native implementation if suitable). The buggy safari implementation is an issue that affects all iPhones and hence this wrapper is useless for cordova based apps.

Anyways, up to you.

@jensarps
Copy link
Owner

jensarps commented Sep 1, 2015

In the end, it comes down to being a Safari issue that the shimIndexedDB property is needed at all (see indexeddbshim/IndexedDBShim#167). So, if that bug weren't there, the shim, if deployed, would always take precedence over the native implementation.

Having that in mind, I have to agree to

if the shim is not needed, the shim should not be exposed

which in turn means that it should be ok to prefer the shim over a native implementation and move responsibility to the developer/deployer; who knows, this way one might be able to use the shim also to outgo IE's buggy IDB impl.

Could you please change your PR to not include the second lookup of shimIndexedDB at the end of the line? Thanks.

Will merge this.

@rubenstolk
Copy link
Contributor Author

Sorry, missed that one, done.

@jensarps
Copy link
Owner

jensarps commented Sep 1, 2015

Great, thanks!

jensarps added a commit that referenced this pull request Sep 1, 2015
Prefer `shimIndexedDB` over `indexedDB`
@jensarps jensarps merged commit 8a67ff7 into jensarps:master Sep 1, 2015
@jensarps
Copy link
Owner

jensarps commented Nov 7, 2015

@rubenstolk Had to change that again, the shim is no more preferred as of 1.6.1. However, you can set the impl preference now yourself: https://github.com/jensarps/IDBWrapper/wiki/Usage#defining-implementation-preference-aka-make-idbwrapper-use-the-shim

I got a mail from a user who was always loading the shim, regardless of what browser was running, and updating to 1.6.0 broke his app on browsers w/o SQLite. Always including the shim is a questionable decision, as is the shim not checking for SQLite existence, but those scenarios seemingly exist and should survive an update of IDBWrapper w/o exploding.

So, sorry for the inconvenience, but I think the impl preference is a reasonable and future-proof way to handle this.

@rubenstolk
Copy link
Contributor Author

Hi @jensarps I like your solution and will implement this accordingly in my situation. Thanks for informing!

@rubenstolk rubenstolk deleted the patch-1 branch November 8, 2015 16:35
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

2 participants