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

Capture a rejected deviceReady correctly #16

Closed
wants to merge 1 commit into from
Closed

Capture a rejected deviceReady correctly #16

wants to merge 1 commit into from

Conversation

zarko-tg
Copy link

Do not throw errors in the console when running on desktop (where cordova is undefined).
It's likely the solution (and the description) for
#2

@thgreasi
Copy link
Owner

If I can recall it correctly, we intended to try to open the database even when the deviceReady gets rejected. How about moving the new catch code to its original position? Would this still result error logs?

@zarko-tg
Copy link
Author

I actually tried keeping the catch where it was but it only resulted with other error logs. You might want to review the totality of using Promise, the code was not the easiest to comprehend.
For example (even though it's a separate topic) in utils.js you have calls to Promise.reject without them being returned.

@thgreasi
Copy link
Owner

thgreasi commented Aug 30, 2016

To be honest, I've been trying to reproduce your error the last two days, but with no "luck"... I tried to reproduce both on the Ionic v1 and Ionic v2 sample projects. It would be great if you could modify one of them so that the error occurs on ionic serve. What version of localforage are you using?

On the other hand, by just reading the PR, having the openDatabasePromise to be always resolved doesn't feel quite right about error handling (even when returning an undefined value).

@zarko-tg
Copy link
Author

test.zip

The minimal test is quite simple really. Just open index.html in your desktop browser (enough to use the file protocol file:///test/index.html) and observe the browser error console.
Only having the localForage-cordovaSQLiteDriver script included and no cordova present (which is normal when for example developing/testing Cordova/Ionic apps in the browser) you get this console error:

Uncaught (in promise) TypeError: function resolve() { [native code] }
called on non-object at resolve (native)

So to wrap up, this PR should solve the immediate problem of inconvenience if accepted and verified by you. But in general you might want to think of improving the related code a bit when it comes to using Promises.

Thanks

@thgreasi
Copy link
Owner

in utils.js you have calls to Promise.reject without them being returned

Thanks for the heads up.
I will check out your test project.

@thgreasi
Copy link
Owner

My first note is that the error occurs only on Chrome and NOT on Firefox. This is probably one of the new async debugging extras of Chrome that logs possibly unhandled Promise rejections.

I removed the Uncaught (in promise) TypeError: function resolve() { [native code] } called on non-object(…) Chrome-only error in v1.4.4 by explicitelly calling resolve() myself.
Interestingly enough, Chrome still errors that Uncaught (in promise) SQLite plugin is not present. even by just having an unused rejected Promise assigned to var openDatabasePromise !!!

PS: it seems that angular's 2 zone.js tried to bring these promise debugging features to all browsers, so the error appears on Firefox as well.

@thgreasi
Copy link
Owner

Just released v1.4.5 that resolved the aforementioned issue by using a function to wait 4 the device ready and retrieve the openDatabase function, while not storing any rejected&unhandled Promise.

Thanks so much for your collaboration on this.
I wouldn't end up with that result without your help.
Sorry for my under-documented code, especially in the cordova-sqlite.js.

@thgreasi thgreasi closed this Aug 31, 2016
@zarko-tg
Copy link
Author

Thanks @thgreasi, I'll have a look and test the latest tomorrow.

p.s. The reason I came to use this neat driver of yours in the first place is because it will help in addressing an issue such as ionic-team/cordova-plugin-wkwebview-engine#28

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