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 wasmJs target #4965

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

IlyaGulya
Copy link

@IlyaGulya IlyaGulya commented Jan 16, 2024

What's impossible for now:

  1. Commonization of WebWorkerDriver can't be done without support for Kotlin arrays interop with wasmJs: https://youtrack.jetbrains.com/issue/KT-57125/Add-interop-with-JavaScript-arrays-to-Kotlin-Wasm

Changes:

  1. Enable wasmJs target in MultiplatformConventions and :driver-test.
  2. Migrate to use kotlin multiplatform default hierarchy template.
  3. Add jsCommon source set containing common code for js and wasmJs targets.
  4. Added jsNativeCommon source set to :runtime module containing common code for native, js, and wasmJs targets.
  5. Enable watchosDeviceArm64 and linuxArm64 because of failing tests.
  6. Support WebWorkerDriver for wasmJs target. Fix all tests to be passing.

@dellisd dellisd self-assigned this Jan 17, 2024
@dellisd
Copy link
Collaborator

dellisd commented Jan 31, 2024

Is the conversion to .gradle.kts required to enable the wasmJs target? If not, it would be best to leave that conversion for a different PR since this one has a lot of files that are just moved and it's more difficult to review the actual changes here.

@IlyaGulya
Copy link
Author

Sure, I will revert this change

@IlyaGulya IlyaGulya force-pushed the feature/wasmJs branch 6 times, most recently from 2c9fe7e to 5c2d9a3 Compare February 5, 2024 07:24
@IlyaGulya
Copy link
Author

@dellisd Hi!
I've rebased on the latest master.
Here are changes since your comment:

  1. Reverted the migration to .gradle.kts.
  2. Made web-worker-driver tests for js and wasmJs common.
  3. Made createWebWorkerDriver helper function which allows creating the default WebWorkerDriver in the jsCommon sourceSet.
  4. Upgraded Kotlin to 2.0.0-Beta3 and KSP to 2.0.0-Beta3-1.0.17

@IlyaGulya
Copy link
Author

IlyaGulya commented Feb 18, 2024

@dellisd Hi!
I've rebased on latest master.
Here're changes since my previous comment:

  1. Upgraded Kotlin to 2.0.0-Beta4 and KSP to 2.0.0-Beta4-1.0.17
  2. Upgraded kotlinx.coroutines to 1.8.0 with wasmJs support
  3. Temporarily added Sonatype repository containing Turbine 1.1.0-SNAPSHOT

@IlyaGulya IlyaGulya force-pushed the feature/wasmJs branch 2 times, most recently from 0f55176 to 8d53f00 Compare February 19, 2024 06:06
@IlyaGulya
Copy link
Author

I'm not sure why, but instrumentation tests are failing on CI.
They're passing locally on my M1 Max.
https://github.com/IlyaGulya/sqldelight/actions/runs/7957378205/job/21719986393

@artemyto
Copy link

artemyto commented Mar 6, 2024

Turbine 1.1.0 with wasmJs support is out

@IlyaGulya
Copy link
Author

Replaced Turbine snapshot with 1.1.0

@IlyaGulya
Copy link
Author

Currently trying to fix tests 🙂

@IlyaGulya IlyaGulya force-pushed the feature/wasmJs branch 6 times, most recently from 49e197c to 672ff65 Compare March 20, 2024 12:49
@IlyaGulya IlyaGulya changed the title [WIP] Support wasmJs target Support wasmJs target Mar 25, 2024
@IlyaGulya IlyaGulya changed the title Support wasmJs target [WIP] Support wasmJs target Mar 25, 2024
@IlyaGulya IlyaGulya force-pushed the feature/wasmJs branch 2 times, most recently from 8838b26 to f3fbc73 Compare March 25, 2024 16:01
@IlyaGulya
Copy link
Author

@dellisd Hi!
All tests are passing. I was able to rollback the kotlin 2.0 upgrade, so I think it can be merged after the code review 🙂
Please take a look when you will have time.

@IlyaGulya IlyaGulya changed the title [WIP] Support wasmJs target Support wasmJs target Mar 25, 2024
Copy link
Collaborator

@dellisd dellisd left a comment

Choose a reason for hiding this comment

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

Overall this looks good, but there are some changes to the public API that I think we should make and I'm a little unclear on why certain parts of the Wasm implementation can't be shared with the Js implementation.

@@ -23,9 +26,11 @@ kotlin {
watchosX64()
watchosArm32()
watchosArm64()
watchosDeviceArm64()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because these aren't directly related to adding Wasm support, I think these would be better split out into their own PR to introduce support for these targets.

Copy link
Author

Choose a reason for hiding this comment

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

Done!
I don't really remember why I've added them. AFAIK, because of some tests failing, but I can't reproduce it now.

testTask {
useKarma {
useChromeHeadless()
}
}
}
}
applyDefaultHierarchyTemplate {
it.common {
it.group("jsCommon") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any need for this to be jsCommon instead of leaving it as the default common?

Copy link
Author

@IlyaGulya IlyaGulya May 26, 2024

Choose a reason for hiding this comment

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

Yeah, you are right, it's unnecessary. My bad, I'll remove this group.


import app.cash.sqldelight.db.SqlDriver

expect fun createWebWorkerDriver(): SqlDriver
Copy link
Collaborator

Choose a reason for hiding this comment

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

The web worker driver is designed to support more than just one worker implementation, so this function should be able to accept a Worker parameter to allow a consumer to define which worker should be loaded.

Copy link
Author

Choose a reason for hiding this comment

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

It is still possible to create WebWorkerDriver with custom Worker.
I think we could just rename this method to createDefaultWebWorkerDriver() instead.

* @property worker The Worker running a SQL implementation that this driver communicates with.
* @see [WebWorkerDriver.fromScriptUrl]
*/
class WebWorkerDriver(private val worker: Worker) : SqlDriver {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which elements of this class (and its jsMain counterpart) can't be lifted into the commonMain implementation? Ideally I think we would have one common implementation of the worker driver, and then expect/actual the web API bits (i.e. Worker) as needed.

Copy link
Author

@IlyaGulya IlyaGulya May 26, 2024

Choose a reason for hiding this comment

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

TBH, most of it 🙁
I have tried to make it as common as possible, but found out that currently there's no way to do it properly.

Most of JS APIs are not common between js and wasmJs.
Current wasmJs interop is quite limited.
It does not support: Array, dynamic in external types. Also, it does not allow arbitrary types which are not extending JsAny to be passed to external functions.
This means that we cannot share WorkerRequest, WorkerResult, WorkerResponse.
Also, we would need to expect/actual quite big portion of Worker API with custom wrappers all over the place.

I will try some more ways to commonise as much as possible and show you in a couple of days, maybe we will find a better solution.

Copy link
Author

Choose a reason for hiding this comment

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

@dellisd I've tried to commonize WebWorkerDriver as much as possible. Please take a look 🙂 ece537a

@hfhbd
Copy link
Collaborator

hfhbd commented May 30, 2024

Do you need Kotlin 2.0 for this change too? I bumped it in this PR #5217, but I will break my PR into smaller ones.

@IlyaGulya
Copy link
Author

IlyaGulya commented May 30, 2024

Do you need Kotlin 2.0 for this change too? I bumped it in this PR #5217, but I will break my PR into smaller ones.

Not really.
But I will be happy to rebase on 2.0 once it's merged 🙂

@tamimattafi
Copy link

Any updates on this?

@dpnkrg
Copy link

dpnkrg commented Jul 26, 2024

Any update on the same.

@sunildhiman90
Copy link

Any update on this please?

@petrstetka
Copy link

I am also interested in wasm support, it would be very nice to have it ready.

@ahna92
Copy link

ahna92 commented Aug 10, 2024

can we get an alpha (early access) version with this change to try it out ? 🙏

@ghasemdev
Copy link

I was developing a multiplatform library and needed to implement a database. After adding SQLDelight, I realized that the plugin causes errors in the wasmjs module, essentially forcing me to remove this module because there's no way to ignore the error and use SQLDelight on other platforms while handling storage differently in wasm. Currently, libraries like Koin, Ktor, ViewModel, and Lifecycle support wasm, and the lack of SQLDelight support is noticeable.

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.

None yet

10 participants