Initial port for wasmJs #170

eygraber · 2024-08-30T05:15:51Z

I'd like to supply a common interface, but at first glance that looks like it might be a lot more work. I started work on commonizing the interface. Tests are currently failing; I am working on that now. Commonizing is done and tests are passing. This solution required having expect/actuals for primitives (so that js and wasmJs have a shared JsAny; see discussion here and elsewhere).

~~I'm trying to rework the API a bit so that none of that has to be exposed. Not sure if it is possible, but I'll give it a try.~~

It should be done now. There's still weirdness to support common code between js and wasmJs, but hopefully the API surface is a little better now, e.g. a lot of the type checking happen when you create a key instead of validating a key after it was created. There are still footguns available to the user, but they would be there anyways (e.g. unsafe casting).

It seems like there is partial support for null keys in js, but the spec seems to suggest that it shouldn't be allowed. I mirrored the null key support in wasmJs for now.

Fixes #157

- I put this separate because it's a lot to have in the file - This would be a good candidate for context parameters when that is available

cedrickcooke · 2024-09-03T16:51:33Z

This is awesome, thank you! I'll start looking at it ASAP

cedrickcooke · 2024-09-04T02:31:36Z

core/src/commonMain/kotlin/Cursor.kt

@@ -1,17 +1,17 @@
-package com.juul.indexeddb


It looks like quite a few files are missing their package directive now. Just gonna leave this one comment to cover all of them.

cedrickcooke · 2024-09-04T02:57:26Z

core/src/commonMain/kotlin/Transaction.kt

+     * This API is delicate. If you're passing in Kotlin objects directly, you're probably doing it wrong.
+     *
+     * Generally, you'll want to create an explicit `external interface` and pass that in, to guarantee that Kotlin
+     * doesn't mangle, prefix, or otherwise mess with your field names.


Now that we have JsAny, I don't think we need the lines about delicate API and external interface. The same applies to all the other overloads of this function, and similar functions like put.

cedrickcooke · 2024-09-04T03:00:21Z

external/src/commonMain/kotlin/Interop.kt

+public expect interface JsAny
+public expect interface UnsafeJsAny : JsAny


What's the difference between JsAny and UnsafeJsAny?

cedrickcooke · 2024-09-04T03:04:42Z

external/src/commonMain/kotlin/Interop.kt

+@PublishedApi
+internal expect fun Array<IDBKey>.unsafeCast(): IDBKey
+
+public expect interface JsAny


Would love to see a doc comment on this explaining it. From my understanding, JsAny is meant as a marker of DB-safe types. Ideally something that includes theexternal interface MyRowType : JsAny use case.

JsAny is really just there so that there can be common code for Js and Wasm. Wasm requires externals to extend JsAny, so that had to be represented in the common code.

I believe Kotlin is working on improving the interop story for Js and Wasm, so this can likely get simplified in the future.

cedrickcooke · 2024-09-04T03:08:14Z

core/src/commonTest/kotlin/AutoIncrementKeyObjectStore.kt

+        database.close()
+        deleteDatabase("auto-increment-keys")


I'm not opposed to losing the test wrapper if there was a real reason it had to go (I'm guessing it didn't play nice with wasm?), but if it's gone there needs to be a try ... finally for these cleanup lines. Otherwise a failing test leaves some garbage behind that can make future test runs (which should be successful) fail. Same goes for the other tests

There was an issue with Wasm, although I have a few ideas about how to make it work.

But worst case scenario I will add the try...finally

cedrickcooke · 2024-09-04T03:16:43Z

Absolutely heroic effort! A few comments, but nothing big/architectural, just some small details.

There's still weirdness to support common code between js and wasmJs, but hopefully the API surface is a little better now, e.g. a lot of the type checking happen when you create a key instead of validating a key after it was created. There are still footguns available to the user, but they would be there anyways (e.g. unsafe casting).

Love the improvements on this front. The footguns are now, at least, more explicit -- it looks a lot harder to accidentally pass something as JsAny than it was as dynamic.

mipastgt · 2024-09-14T11:52:19Z

Any progress on this front?

eygraber · 2024-09-15T01:04:47Z

I've been distracted, but I hope to get back to this in the coming weeks.

twyatt

This is a bit outside my wheelhouse, but I'll try to review the code for readability/understandability more so than correctness of interacting with IndexedDB.
Just a couple comments so far. Overall, as @cedrickcooke mentioned, what an amazing effort on this! Thanks!

twyatt · 2024-09-18T22:59:36Z

README.md

 [![Slack](https://img.shields.io/badge/Slack-%23juul--libraries-ECB22E.svg?logo=data:image/svg+xml;base64,PHN2ZyB2aWV3Qm94PSIwIDAgNTQgNTQiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PGcgZmlsbD0ibm9uZSIgZmlsbC1ydWxlPSJldmVub2RkIj48cGF0aCBkPSJNMTkuNzEyLjEzM2E1LjM4MSA1LjM4MSAwIDAgMC01LjM3NiA1LjM4NyA1LjM4MSA1LjM4MSAwIDAgMCA1LjM3NiA1LjM4Nmg1LjM3NlY1LjUyQTUuMzgxIDUuMzgxIDAgMCAwIDE5LjcxMi4xMzNtMCAxNC4zNjVINS4zNzZBNS4zODEgNS4zODEgMCAwIDAgMCAxOS44ODRhNS4zODEgNS4zODEgMCAwIDAgNS4zNzYgNS4zODdoMTQuMzM2YTUuMzgxIDUuMzgxIDAgMCAwIDUuMzc2LTUuMzg3IDUuMzgxIDUuMzgxIDAgMCAwLTUuMzc2LTUuMzg2IiBmaWxsPSIjMzZDNUYwIi8+PHBhdGggZD0iTTUzLjc2IDE5Ljg4NGE1LjM4MSA1LjM4MSAwIDAgMC01LjM3Ni01LjM4NiA1LjM4MSA1LjM4MSAwIDAgMC01LjM3NiA1LjM4NnY1LjM4N2g1LjM3NmE1LjM4MSA1LjM4MSAwIDAgMCA1LjM3Ni01LjM4N20tMTQuMzM2IDBWNS41MkE1LjM4MSA1LjM4MSAwIDAgMCAzNC4wNDguMTMzYTUuMzgxIDUuMzgxIDAgMCAwLTUuMzc2IDUuMzg3djE0LjM2NGE1LjM4MSA1LjM4MSAwIDAgMCA1LjM3NiA1LjM4NyA1LjM4MSA1LjM4MSAwIDAgMCA1LjM3Ni01LjM4NyIgZmlsbD0iIzJFQjY3RCIvPjxwYXRoIGQ9Ik0zNC4wNDggNTRhNS4zODEgNS4zODEgMCAwIDAgNS4zNzYtNS4zODcgNS4zODEgNS4zODEgMCAwIDAtNS4zNzYtNS4zODZoLTUuMzc2djUuMzg2QTUuMzgxIDUuMzgxIDAgMCAwIDM0LjA0OCA1NG0wLTE0LjM2NWgxNC4zMzZhNS4zODEgNS4zODEgMCAwIDAgNS4zNzYtNS4zODYgNS4zODEgNS4zODEgMCAwIDAtNS4zNzYtNS4zODdIMzQuMDQ4YTUuMzgxIDUuMzgxIDAgMCAwLTUuMzc2IDUuMzg3IDUuMzgxIDUuMzgxIDAgMCAwIDUuMzc2IDUuMzg2IiBmaWxsPSIjRUNCMjJFIi8+PHBhdGggZD0iTTAgMzQuMjQ5YTUuMzgxIDUuMzgxIDAgMCAwIDUuMzc2IDUuMzg2IDUuMzgxIDUuMzgxIDAgMCAwIDUuMzc2LTUuMzg2di01LjM4N0g1LjM3NkE1LjM4MSA1LjM4MSAwIDAgMCAwIDM0LjI1bTE0LjMzNi0uMDAxdjE0LjM2NEE1LjM4MSA1LjM4MSAwIDAgMCAxOS43MTIgNTRhNS4zODEgNS4zODEgMCAwIDAgNS4zNzYtNS4zODdWMzQuMjVhNS4zODEgNS4zODEgMCAwIDAtNS4zNzYtNS4zODcgNS4zODEgNS4zODEgMCAwIDAtNS4zNzYgNS4zODciIGZpbGw9IiNFMDFFNUEiLz48L2c+PC9zdmc+&labelColor=611f69)](https://kotlinlang.slack.com/messages/juul-libraries/)

 # Kotlin IndexedDB

-A wrapper around [IndexedDB] which allows for access from Kotlin/JS code using `suspend` blocks and linear, non-callback based control flow.
+A wrapper around [IndexedDB] which allows for access from Kotlin/JS or Kotlin/WasmJs code using `suspend` blocks and linear, non-callback based control flow.


nitpick: typically "Kotlin/Wasm" is used (without the "Js" attached).

Suggested change

A wrapper around [IndexedDB] which allows for access from Kotlin/JS or Kotlin/WasmJs code using `suspend` blocks and linear, non-callback based control flow.

A wrapper around [IndexedDB] which allows for access from Kotlin/JS or Kotlin/Wasm code using `suspend` blocks and linear, non-callback based control flow.

I prefer the original wording because it is more precise. The use of IndexedDB is tied to the browser and thus to the wasmJS target and not WASM in general.

I did that to differentiate it from wasmWasi, although I guess the fact that it's a browser API should be enough to differentiate implicitly.

twyatt · 2024-09-19T03:18:50Z

external/src/jsMain/kotlin/Interop.kt

+import kotlin.js.unsafeCast
+import kotlin.js.unsafeCast as jsUnsafeCast


Curious the reason for the import aliasing?
Also, why both unsafeCast import and the alias for it too?

Could be a mistake, I'll double check.

apolostudio · 2024-10-23T19:10:36Z

pinging this!

eygraber · 2024-10-23T20:01:05Z

Sorry, haven't had much free time to get back to this. Hopefully will be able to soon.

eygraber · 2024-11-10T23:14:20Z

Update: real life continues to get in the way of addressing comments 😞

I haven't forgotten about it, and will come back to it as soon as I'm able, but if anyone wants to pick up where I left off, feel free 😄

Initial port for wasmJs

7c2f77d

eygraber requested review from cedrickcooke and a team as code owners August 30, 2024 05:15

eygraber requested a review from twyatt August 30, 2024 05:15

eygraber force-pushed the wasm branch 6 times, most recently from 4e2d6d5 to d72f896 Compare September 2, 2024 04:28

eygraber mentioned this pull request Sep 2, 2024

FeatureRequest: Add the ability to use Indexed DB for JS and WasmJs xxfast/KStore#119

Open

eygraber force-pushed the wasm branch from d72f896 to e4c74e9 Compare September 2, 2024 19:35

Commonize

bbd0d59

eygraber force-pushed the wasm branch from e4c74e9 to bbd0d59 Compare September 2, 2024 19:40

Convenience overloads for add and put

f414d45

- I put this separate because it's a lot to have in the file - This would be a good candidate for context parameters when that is available

cedrickcooke reviewed Sep 4, 2024

View reviewed changes

cedrickcooke added the major Changes that should bump the MAJOR version number label Sep 4, 2024

cedrickcooke mentioned this pull request Sep 5, 2024

Logging Support #171

Open

twyatt reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial port for wasmJs #170

Initial port for wasmJs #170

eygraber commented Aug 30, 2024 •

edited

Loading

cedrickcooke commented Sep 3, 2024

cedrickcooke Sep 4, 2024

cedrickcooke Sep 4, 2024

cedrickcooke Sep 4, 2024

cedrickcooke Sep 4, 2024

eygraber Sep 19, 2024

cedrickcooke Sep 4, 2024

eygraber Sep 19, 2024

cedrickcooke commented Sep 4, 2024

mipastgt commented Sep 14, 2024

eygraber commented Sep 15, 2024

twyatt left a comment

twyatt Sep 18, 2024

mipastgt Sep 19, 2024

eygraber Sep 19, 2024

twyatt Sep 19, 2024

eygraber Sep 19, 2024

apolostudio commented Oct 23, 2024

eygraber commented Oct 23, 2024

eygraber commented Nov 10, 2024

		public expect interface JsAny
		public expect interface UnsafeJsAny : JsAny

	A wrapper around [IndexedDB] which allows for access from Kotlin/JS or Kotlin/WasmJs code using `suspend` blocks and linear, non-callback based control flow.
	A wrapper around [IndexedDB] which allows for access from Kotlin/JS or Kotlin/Wasm code using `suspend` blocks and linear, non-callback based control flow.

		import kotlin.js.unsafeCast
		import kotlin.js.unsafeCast as jsUnsafeCast

Initial port for wasmJs #170

Are you sure you want to change the base?

Initial port for wasmJs #170

Conversation

eygraber commented Aug 30, 2024 • edited Loading

cedrickcooke commented Sep 3, 2024

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

cedrickcooke commented Sep 4, 2024

mipastgt commented Sep 14, 2024

eygraber commented Sep 15, 2024

twyatt 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

apolostudio commented Oct 23, 2024

eygraber commented Oct 23, 2024

eygraber commented Nov 10, 2024

eygraber commented Aug 30, 2024 •

edited

Loading