-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(android): implement async Ti.Database.DB methods #10920
Conversation
|
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Show resolved
Hide resolved
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Outdated
Show resolved
Hide resolved
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Show resolved
Hide resolved
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Show resolved
Hide resolved
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Show resolved
Hide resolved
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Outdated
Show resolved
Hide resolved
android/modules/database/src/java/ti/modules/titanium/database/TiDatabaseProxy.java
Outdated
Show resolved
Hide resolved
* @param parameters Parameters for `query` | ||
*/ | ||
@Kroll.method | ||
public void executeAsync(final KrollFunction callback, final String query, final Object... parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in JS do we actually have to pass in the callback as the first argument here, like so?
const db = Ti.Database.open('whatever');
db.executeAsync(result => { Ti.API.info(result); }, 'SELECT * FROM TABLE WHERE name = ?', myName);
Because if so, that is very odd and we should avoid it if possible. I'm assuming the use of the varargs for parameters here is why the callback needed to come first? Can we re-arrange it so the callback is the last JS arg? If we can't "natively" with our binding layer, we could add JS code to basically do the swap under the hood in the same way we do some hacking of say Ti.App.Properties here: https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/app/src/js/properties.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second, it's also not idiomatic to have the async variant have the extra name suffix.
Assuming we can hack around the arguments in JS or whatever, why not just use the same method name and if the last argument is a callback function then treat it as async, otherwise sync?
I suppose that boxes us in if we want to return a Promise...
Maybe introduce a db.executeSync, db.executeAsync where they are explicitly sync/async, have the existing method do either sync or async based on if callback is passed in, and spit out a deprecation warning if no callback is passed to it (basically saying, hey in the future this will return a Promise - so if you want sync behavior use db.executeSync)? I think at some point we want to remove the default sync nature of the method and push people towards async naturally - and eventually support Promises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've modified the parameters so the callback is the last parameter, we need to keep Ti.Database.execute()
to not cause breaking changes.
Ti.Database.DB.executeAsync(query, parameters[], result => { ... });
results[] = Ti.Database.DB.executeAll(queries[]);
Ti.Database.DB.executeAllAsync(queries[], results[] => { ... });
I'd prefer to introduce as few API changes possible until we make the jump to Promise based APIs in the near future. I didn't intend to refactor all of our Ti.Database.DB APIs with this PR.
I think these APIs allow the flexibility to do the primary things, including transactions if the user chooses to do so. We should create a ticket to refactor the methods in 9.0.0
?
* @param queries SQL queries to execute on database. | ||
*/ | ||
@Kroll.method | ||
public Object[] executeAll(final String[] queries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look to something like this node sqlite3 package to consider how they've approached their API: https://github.com/mapbox/node-sqlite3/wiki/API#database
Maybe not introduce prepare/Statements, but having explicit methods to say whether you want to run something without caring about the result, or wanting the first result, or calling the callback with the batch results once vs calling for each row seems useful to me.
Note that there is also this package which does explicitly run sync intentionally for performance: https://github.com/JoshuaWise/better-sqlite3/blob/master/docs/api.md
I recall @jquick-axway mentioning usage of transactions helped performance significantly. Would we wrap this batch of queries in a transaction for the user? Would we expect them to include their own transaction management statements in this batch?
'use strict'; | ||
const should = require('./utilities/assertions'); | ||
|
||
describe.only('Titanium.Database', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ tests/Resources/ti.database.2.addontest.js line 13 – Unexpected exclusive mocha test. (mocha/no-exclusive-tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
My only remaining feedback here is that the batch execute methods only take strings and there's no way to do the string + params style you'd do in a single query. I think that'd be a fairly common annoyance here, and you may want to consider being more expansive on how to handle elements in the queries
array so it's a Object[]
and could hold either a single String
in each entry, or another Object[]
with first element the sql String
, the remaining the varargs params.
Something like:
const queries = [
'DELETE FROM testTable',
['INSERT INTO testTable (text, number) VALUES (?, ?)', testName, testNumber]
]
FR Passed.
Studio Ver: 5.1.3.201906102126 |
@garymathews , Its complaining about unit tests. Does it need any ? |
Ti.Database.DB
methods for executing SQL queries.Ti.Database.DB.executeAsync(query, parameters[], callback)
Ti.Database.DB.executeAllAsync(queries[], callback)
JIRA Ticket