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

Add Wasm Support for kotlin-document-store #47

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

Conversation

ApoloApps
Copy link

@ApoloApps ApoloApps commented Jan 14, 2025

Basically copying js source set code and adapting it to WasmJs. Since dynamic is not available in WasmJs and Promise requires a type that extends JsAny, ive had to replace all Any's and Array's and String's in Promise scope to Wasm Js versions of them (JsArray, JsString,JsAny) while allowing nullability. Since Unit is not a valid JsAny, ive had to replace it with JsAny? since i think there is no other way.
Tested in my browser and it worked. Let me know if you want any changes! (ive tried putting all in a webmain source set but definedExternally, dynamic and needing the Js variants in Wasm, ive given up and let every source set have their own impls)
(i think tests hang with promise, so idk if remove them or not)
image

@lamba92 lamba92 self-requested a review January 19, 2025 21:35
@lamba92 lamba92 assigned lamba92 and unassigned lamba92 Jan 19, 2025
Copy link
Owner

@lamba92 lamba92 left a comment

Choose a reason for hiding this comment

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

Amazing work 🚀 Thank you for the PR!

It might be worth to commonize idb-keyval symbols only in webMain and use those symbols to implement the common APIs. This would drastically reduce the duplicated code. wdyt?

Also, do not forget to run on your machine before pushing changes ./gradlew ktlintFormat to format Kotlin files automatically so that the PR can pass the Linting check

stores/browser/build.gradle.kts Outdated Show resolved Hide resolved
jsMain {

val webMain by creating {
dependsOn(commonMain.get())
dependencies {
api(npm("idb-keyval", "6.2.1"))
Copy link
Owner

Choose a reason for hiding this comment

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

Amazing! You can use NPM libraries in wasmJs as well?

Copy link
Author

Choose a reason for hiding this comment

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

yes! it can be used but they need to be compatible with browser (which keyval is)

@ApoloApps
Copy link
Author

ApoloApps commented Jan 28, 2025

It might be worth to commonize idb-keyval symbols only in webMain and use those symbols to implement the common APIs. This would drastically reduce the duplicated code. wdyt?

Thats the problem, ive tried commonizing all idb-keyval symbols but definedExternally cannot be expect actualised in webMain cause they have different implementations is js and wasm js and so external keyval classes must be in each specific source set. Promise has the same problem because it extends different implementations in each platform (Ive tried expect actualising but its currently not possible due to wasm extending JsAny and js does not). I've seen a lot of libraries that have the same problem and just copy the full implementation due to these restrictive problems with wasmjs

Edit: see https://youtrack.jetbrains.com/issue/KT-74780/KJS.-Common-definedExternally-for-KJS-and-KWasmJS

@lamba92
Copy link
Owner

lamba92 commented Feb 5, 2025

It might be worth to commonize idb-keyval symbols only in webMain and use those symbols to implement the common APIs. This would drastically reduce the duplicated code. wdyt?

I mean, just a top level functions such as expect fun getValue(key: String): String. No need to expect the actual ones from idb-keyval. Then for each platform, you connect to the actual one.

You can express the definedExternally with null with something like:

expect fun aFunction(aParameter: String? = null)
// ...
actual fun aFunction(aParameter: String?) {
  if (aParameter != null) externalJsFunction(aParameter)
  else externalJsFunction()
}

wdyt?

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