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

(#1191) - Lazy openDB() #1192

Closed
wants to merge 4 commits into from
Closed

Conversation

slaskis
Copy link
Contributor

@slaskis slaskis commented Dec 29, 2013

To fix #1191 this PR makes the openDatabase check lazy so it will be able to find the sqlite plugin.

@calvinmetcalf
Copy link
Member

we probably don't need to worry about somebody not passing arguments, so we can skip the arguments length check. we also want to use as context for the apply as whatever it turns out to be, i.e. window, window. sqlitePlugin or navigator.sqlitePlugin. lastly we want to return the results of the function, not the function itself which means we would probably need a separate check for validity.

@slaskis
Copy link
Contributor Author

slaskis commented Dec 29, 2013

That's actually how I did it first (before a6c5143), but then I suspected it might throw in the valid() check if it actually called the openDatabase function.

@calvinmetcalf
Copy link
Member

we can actually probably revert the validity test to what it was before I added the sqlite plugin

@slaskis
Copy link
Contributor Author

slaskis commented Dec 30, 2013

Allright, I went back to the proper scope and a more explicit validity test, like it was before the sqlite plugin check, but more...

}
return null;
Copy link
Member

Choose a reason for hiding this comment

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

it can just return undefined, aka remove this line

@calvinmetcalf
Copy link
Member

I rebased, squashed the commits into 1 and fixed the commit message merged it, going forward it would be helpful if you could make sure you have only one commit and that the message be in the format '(#issue number) - message.' If you aren't familiar enough with git to be able to do that to a commit like this, then just do the pull like this one and I can walk you through fixing it.

Seriously though thanks for this patch.

@calvinmetcalf
Copy link
Member

8f0aa9c

@slaskis
Copy link
Contributor Author

slaskis commented Jan 2, 2014

No problem @calvinmetcalf, I'm quite familiar with git but I figured I'd squash when "LGTM".

Anyway glad I can help out, really like the way PouchDB is going right now.

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.

Cordova cannot find the sqlite plugin
2 participants