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 for browser platform (with WebSQL). #575

Closed
wants to merge 3 commits into from

Conversation

rafw87
Copy link

@rafw87 rafw87 commented Oct 22, 2016

This is a simple implementation of browser platform, using WebSQL.
It doesn't have implementation of .close() and .deleteDatabase(), these methods are calling success() but are doing nothing (except writing warning to log). Due of it, selfTest is passing only once and reguires to clear database manually.
EDIT: second commit implements .deleteDatabase() by deleting all tables - it's enough to fix selfTest.

@brodycj
Copy link
Contributor

brodycj commented Oct 25, 2016

Hi @rafw87 thanks for the contribution. Did you run it against the full test suite?

Looking at the changes I don't think it will pass the error handling tests. Please correct me if I am mistaken somehow. To be honest I was considering a much different approach.

@rafw87
Copy link
Author

rafw87 commented Oct 25, 2016

I checked this error handling tests and I see that most of errors are caused by different error messages etc than expected, for example:
Error: Expected 'Cannot read property 'toString' of undefined' to match /Unable to get property 'toString' of undefined or null reference/.

I see that there are different expectations for different environments (Windows, Android, WP8), so maybe it should be done that way for browser platform (expectations like for isWebSql)?

@rafw87
Copy link
Author

rafw87 commented Oct 25, 2016

Hi,
I'll check it later today. Both CI jobs passed, I assume that full test suite is something else?

It's possible that some cases are not handled properly. I didn't spend so much time for this platform, it was rather a quick change to have code in my project more clean. I decided to share it "as-is", maybe it will be useful for someone.

Thanks for this plugin:)

-------- Oryginalna wiadomość --------
Od: Chris Brody notifications@github.com
Data:25.10.2016 10:21 (GMT+01:00)
Do: litehelpers/Cordova-sqlite-storage Cordova-sqlite-storage@noreply.github.com
DW: rafw87 raf_w87@wp.pl, Mention mention@noreply.github.com
Temat: Re: [litehelpers/Cordova-sqlite-storage] Support for browser platform (with WebSQL). (#575)
Hi @rafw87 thanks for the contribution. Did you run it against the full test suite?

Looking at the changes I don't think it will pass the error handling tests. Please correct me if I am mistaken somehow. To be honest I was considering a much different approach.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@brodycj
Copy link
Contributor

brodycj commented Oct 25, 2016

Both CI jobs passed, I assume that full test suite is something else?

The CI jobs test the Android and iOS platform implementations. There is no CI for the browser platform since it would be new. (Also there is currently no CI for Windows.)

I decided to share it "as-is", maybe it will be useful for someone.

Thanks for sharing. It was definitely very interesting to try running it.

I think it will take a very different approach to pass some of the more difficult tests including nested transactions, error handling, and error recovery. I will work on this when I get a chance. Discussion will be continued in #297.

Thanks for this plugin:)

You are welcome)

@brodycj
Copy link
Contributor

brodycj commented Feb 9, 2017

Closing in favor of PR #576

@brodycj brodycj closed this Feb 9, 2017
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.

2 participants