Implement a JS driver #1486

tokou · 2019-10-27T20:27:34Z

This is a tentative implementation of a JS driver. It uses sql.js as it looked like the most used and efficient solution.

This is still a WIP and will not pass CI.

The implementation is done.
I also added a web sample (and fixed the other samples). (Run ./gradlew :sample:web:jsBrowserWebpack and then serve the sample/web/build/distributions/index.html file)

The main issue I have is with tests. I duplicated all the tests and adapted them (quite naively).

The database can only be created in an asynchronous fashion in sql.js. This requires adapting all the tests to run with async code.

In the duplicated tests, I implemented the workaround descibed here to run async tests. However there's still an issue as @BeforeTest and @AfterTest are not handled properly as reported here.

Edit: I just submitted a PR in order to fix this and make possible asynchronous beofre and after methods.

Tests in :drivers:sqljs-driver:jsTest pass but they are duplicated.
Some tests in :sample:common:jsTest fail because I don't think it's possible to ignore common tests for a particular target. I duplicated them and fixed them for async code and they pass.

I also duplicated the runTest method in a couple of files for now.

What approach do you recommend for fixing the tests ?

Also, I am not very familiar with publishing of artifacts. I'm sure some steps are missing or maybe wrong to ensure the proper publishing of the JS driver and also some CI steps maybe.

Fixes #1350
Fixes #1294
Closes #1302

AlecKazakova · 2019-12-11T16:16:19Z

drivers/sqljs-driver/src/jsMain/kotlin/com/squareup/sqldelight/drivers/sqljs/sqljs.SqlJs.kt

+package com.squareup.sqldelight.drivers.sqljs
+
+import org.khronos.webgl.Uint8Array
+import kotlin.js.*


nit no * imports, you can search for star imports in intellij preferences to turn off

AlecKazakova · 2019-12-11T16:18:55Z

drivers/sqljs-driver/src/jsMain/kotlin/com/squareup/sqldelight/drivers/sqljs/sqljs.SqlJs.kt

+
+external interface QueryResults {
+    var columns: Array<String>
+    var values: Array<Array<dynamic /* Number | String | Uint8Array | Nothing? */>>


how does this work O_O

Not sure what you mean
You declare the interface you expect from the JS lib, and then you can use it from Kotlin with type checking etc

AlecKazakova · 2019-12-11T16:26:13Z

drivers/sqljs-driver/src/jsMain/kotlin/com/squareup/sqldelight/drivers/sqljs/sqljs.kt

+    return js("new (Function.prototype.bind.apply(type, argsArray))")
+}
+
+suspend fun initSql(config: Config? = js("{}")): SqlJsStatic = initSqlJs(config).await()


is there a way we can remove the dependency on coroutines from the driver? It should ideally be all blocking straightforward APIs that we can wrap in coroutines if necessary

We would have to expose this as a Promise, which I think is fine. The consumer could then await() using the built-in extension for it if they're using coroutines.

yea I like that more

does Promise have a normal blocking await()? if so you could probably just use that then in your tests @tokou to get around the issues you're currently seeing

No. You would have to do something like

@Test fun myTest() = initDb().then { test stuff }

which returns the Promise to the test runner which it will then wait on.

Done, indeed it's much better now

AlecKazakova · 2019-12-11T16:27:33Z

README.md

+```kotlin
+val sql = initSql()
+val db = sql.Database()
+val driver: SqlDriver = JsSqlDriver(db)


can the constructor for JsSqlDriver() just initSql and get the database itself instead of doing these three steps explicitly? Or is the expectation that consumers would use the results of initSql and Database() multiple times

I added some helper functions to initialize the driver directly

AlecKazakova · 2019-12-11T16:28:33Z

this is impressive

I'm going to update the sample code to 1.2.0 today and then we'll need to rebase this pr on that. It should at least clean up the diff that amount. Really love that you also created a sample for the web side, thank you

AlecKazakova · 2019-12-11T16:32:17Z

With regard to publishing we're going to do the kotlin 1.3.60 release, then merge all these new drivers for windows mac and js then fix the publishing/travis story because its a nightmare, then do another release

so it might be a while before this all gets released because theres a lot of work that has to go into it

JakeWharton

Overall great stuff 👍

JakeWharton · 2019-12-11T16:25:14Z

drivers/sqljs-driver/build.gradle

@@ -0,0 +1,79 @@
+apply plugin: 'org.jetbrains.kotlin.multiplatform'


Is there any reason to use multiplatform in this module instead of only the js plugin?

Hmm no, I just switched to the js plugin

JakeWharton · 2019-12-11T16:26:33Z

drivers/sqljs-driver/build.gradle

+    dependsOn 'publishJsPublicationToTestRepository'
+  }
+  // NOTE: We do not alias uploadArchives because CI runs it on Linux and we only want to run it on Mac OS.
+  //tasks.register('uploadArchives').dependsOn('publishKotlinMultiplatformPublicationToMavenRepository')


Don't think we need this commented out since it should work on all host OSs.

I removed all publishing related code for now, as per Alec's comment

JakeWharton · 2019-12-11T16:31:59Z

drivers/sqljs-driver/build.gradle

+kotlin {
+
+  js {
+    useCommonJs()


I'm pretty sure this prevents use in a module system and we should be using UMD (since Kotlin can't emit ES6 module files).

JakeWharton · 2019-12-11T16:33:31Z

drivers/sqljs-driver/src/jsTest/kotlin/com/squareup/sqldelight/drivers/sqljs/JsDriverTest.kt

+        tearDown()
+    }
+
+    @JsName("SqlResultSet_getters_return_null_if_the_column_values_are_NULL")


You can just name the tests like this directly. They only run on JS, so there's no point duplicating them.

tokou · 2020-02-04T16:52:31Z

@AlecStrong Just rebased on the latest master.

Everything seems to work fine (ran ./gradlew clean :drivers:sqljs-driver:test :sample:common:jsTest :sample:web:browserWebpack)
stately does not provide a js for now so running ./gradlew :runtime:jsTest will fail (along with gradle sync in IntelliJ)

AlecKazakova · 2020-02-04T16:55:11Z

i thought stately was only a dep of the native artifact - interesting that it fails the js runtime compilation

tokou · 2020-02-04T16:57:00Z

stately-collection is a dependency on the commonTest source set. See here

tokou · 2020-02-04T18:09:29Z

I could simply replace the class with an expect/actual if you want and remove the dependency on stately in common

AlecKazakova · 2020-02-04T18:09:56Z

if possible i think that makes sense

tokou · 2020-02-04T21:01:50Z

I fixed the CI and updated the README
The only thing left is the publishing of the js driver I guess

AlecKazakova · 2020-02-05T18:24:46Z

dope, will review shortly. We can figure out publishing later

AlecKazakova · 2020-02-06T13:25:42Z

sample/common/src/commonTest/kotlin/com/example/sqldelight/hockey/SchemaTest.kt

@@ -4,6 +4,7 @@ import kotlin.test.Test
 import kotlin.test.assertTrue

 class SchemaTest : BaseTest() {
+  /*


does this still need to be commented out?

No. I missed these. Just pushed a fix. There's no way do disable running common tests on a specific target apparently so I duplicated the tests (the alternative would be to expect a boolean from each target to decide if we should run the test or not).

AlecKazakova · 2020-02-07T14:46:17Z

huge contribution. thanks for doing this

JakeWharton · 2020-02-13T01:44:10Z

runtime/build.gradle

  targets {
    targetFromPreset(presets.jvm, 'jvm')
-    targetFromPreset(presets.js, 'js') {
-      tasks.getByName(compilations.main.compileKotlinTaskName).kotlinOptions {
-        moduleKind = 'umd'


Why was this removed?

It was replaced with the js block to define the target
Sorry I'm not sure why I did this

AlecKazakova approved these changes Dec 11, 2019

View reviewed changes

JakeWharton reviewed Dec 11, 2019

View reviewed changes

AlecKazakova force-pushed the master branch 21 times, most recently from 7865f4c to 03c08a2 Compare January 23, 2020 17:43

tokou force-pushed the sqljs-driver branch from 921aa8d to 79f4df0 Compare January 31, 2020 12:39

tokou added 4 commits February 4, 2020 16:26

Add JS implementation with duplicated tests

a0482b0

Fix sample by updating it to sqldelight 1.2.0

aa2e427

Add web sample with duplicated tests

8e2c5a8

Make classes private

1e7c5c8

tokou added 7 commits February 4, 2020 17:04

Fix sample

23b660c

Use js plugin instead of mpp

22fee1b

Switch to UMD instead of common js modules

a41e6c4

Switch sample to js plugin instead of mpp

3f39454

Remove unnecessary workaround

e1bc749

Update web sample dependencies

92b8466

Fix tests

1e33351

tokou force-pushed the sqljs-driver branch from 79f4df0 to 1e33351 Compare February 4, 2020 16:49

Remove common dependency on stately-collections

d47402c

tokou force-pushed the sqljs-driver branch from b088b9e to d47402c Compare February 4, 2020 19:27

tokou added 4 commits February 4, 2020 20:41

Bump mocha timeout

b93267b

Remove useless runtime

f171817

Bump mocha timeout

a6ece01

Update README

7ef6b2b

AlecKazakova reviewed Feb 6, 2020

View reviewed changes

AlecKazakova approved these changes Feb 6, 2020

View reviewed changes

Fix sample common tests

a57710c

AlecKazakova merged commit 43ca376 into sqldelight:master Feb 7, 2020

JakeWharton reviewed Feb 13, 2020

View reviewed changes

tokou deleted the sqljs-driver branch March 31, 2020 12:50

samhill303 mentioned this pull request Jul 6, 2020

No JS target? touchlab/KaMPKit#116

Closed

martinbonnin mentioned this pull request Oct 22, 2021

Javascript SQLite support apollographql/apollo-kotlin#3442

Open

		@@ -0,0 +1,79 @@
		apply plugin: 'org.jetbrains.kotlin.multiplatform'

Implement a JS driver #1486

Implement a JS driver #1486

Conversation

tokou commented Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlecKazakova commented Dec 11, 2019

AlecKazakova commented Dec 11, 2019

JakeWharton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokou commented Feb 4, 2020

AlecKazakova commented Feb 4, 2020

tokou commented Feb 4, 2020

tokou commented Feb 4, 2020

AlecKazakova commented Feb 4, 2020

tokou commented Feb 4, 2020

AlecKazakova commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlecKazakova commented Feb 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokou commented Oct 27, 2019 •

edited

Loading