-
Notifications
You must be signed in to change notification settings - Fork 57
Kotlin serialization migration #190
Kotlin serialization migration #190
Conversation
# Conflicts: # .gitignore # build.gradle # src/main/kotlin/kweb/Element.kt # src/main/kotlin/kweb/html/Document.kt
…n_Serialization_Migration # Conflicts: # build.gradle # src/main/kotlin/kweb/Element.kt # src/main/kotlin/kweb/ElementCreator.kt # src/main/kotlin/kweb/WebBrowser.kt # src/main/kotlin/kweb/client/Server2ClientMessage.kt # src/main/kotlin/kweb/html/fileUpload/FileFormInput.kt # src/main/kotlin/kweb/util/misc.kt
Re replacing |
This branch is a mess at the moment. There are conflicts with master, and It doesn't even compile at the moment. But, I wanted you to see what it looks like, to make sure we'll be happy going this direction. Also, I'm having an issue with 2 functions in Element.kt These are setAttributeRaw() and setAttribute(). I'm not sure how far up the ladder I should be changing fun setAttribute(name: String, oValue: KVal): Element { Fixing setAttribute() is a little trickier though. If I overload setAttributeRaw(), we might not even need to change setAttribute(). But, if we make setAttributeRaw() take a JsonElement, than setAttribute will have to convert oValue to JsonElement. And it can't do that. Unless, we rewrite KVal to use JsonElement instead of a generic. Which, seems like it could be appealing. I don't know how much work that would take, or if it is a direction we want to go. But, I think it has potential. |
I think both setAttribute() and setAttributeRaw() should take a JsonPrimitive as a value (or a KVal in the case of setAttribute()), since in practice this is what an attribute value will be (a JavaScript primitive). If we do this then I think it means we can get rid of setAttributeRaw() and just rely on overloading to decide whether the value is a So, to be explicit, the new functions would be: fun setAttribute(name : String, value : JsonPrimitive) { .. }
fun setAttribute(name : String, value : KVal<JsonPrimitive>) { .. }
I'm not sure I understand the issue here, is it solved if we just decide that the value must be a JsonPrimitive as discussed above? |
The issue I was having is resolved by using JsonPrimitive like in your example there. I am wondering though if we should rewrite Kval to have a JsonElement as a value, instead of using the Template. The template is starting to feel a little unnecessary to me. I could be wrong though. Also, my 2 commits here, have gotten me into a much better place. Stuff is still broken though. Kweb.kt's respond function is now erroring with "Expected Begin Object but got String", which is the opposite of what it used to error with. I'm sure I can fix that with a bit more modification. I've also run the todo app. The event still isn't working with the add item button. And, the page no longer renders correctly. It looks like it is missing some styling. Let me know if anything in the commits jumps out at you that would cause that to happen. It's probably pretty simple. |
I'm not sure I understand what you mean by "Template" in this context, could you elaborate?
I'll take a look. |
@@ -27,7 +65,7 @@ class CookieReceiver(val receiver: WebBrowser) { | |||
arguments.add(domain) | |||
} | |||
|
|||
receiver.callJsFunction("docCookies.setItem({});", arguments.joinToString(separator = ", ")) | |||
receiver.callJsFunction("docCookies.setItem({});", JsonPrimitive(arguments.joinToString(separator = ", "))) |
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.
Not sure, is this definitely what setItem() is expecting (a string containing a comma-separated list)?
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.
No it is not. This is a mistake I made awhile ago and it slipped through the cracks. It's an easy fix though.
Sorry, "Template" was the wrong word. I think I should have said "Generic". Template is a C++ word. But, what I'm saying is, maybe Kval should go from
to
|
Hmm, I'm not sure I see the advantage of that and it would significantly reduce the utility of KVals and KVars. In particular, when using a KVar/KVal with |
I said JsonElement, not JsonPrimitive. The idea is that for functions like setAttribute(), we could do |
We have 4 uses of gson.fromJson() that are tripping me up. I'm not 100% sure how they work and what they do, and I don't want to accidentally break something by changing them incorrectly. These 4 uses are in StorageReceiver And JqueryCore events The last one is a plugin, and per a previous discussion, we could probably just remove that one. For
That doesn't break anything in the tests, or either demo app I've been using. But, I'm not convinced it will work in as many places as the old version. And I'm less clear on how to handle the code block in |
…o 1.2.0, because there seems to be a bug there.
…xtra quotes being added to Strings from the client.
…ion_Migration # Conflicts: # build.gradle # src/main/kotlin/kweb/Element.kt # src/main/kotlin/kweb/Kweb.kt
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.
Looks great, just some minor nitpicks.
build.gradle
Outdated
@@ -45,17 +45,18 @@ dependencies { | |||
compile 'org.apache.commons:commons-lang3:3.11' | |||
compile 'commons-io:commons-io:2.8.0' | |||
compile 'org.jsoup:jsoup:1.13.1' | |||
compile 'com.github.kwebio:shoebox:0.4.12' | |||
compile 'com.github.kwebio:shoebox:0.4.5' |
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.
Current shoebox version is 0.4.35 (I had to do a bunch of releases while trying to fix that problem :( ).
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.
This was a typo. I'm surprised Kweb still built and ran when asking for a non existent version of Shoebox.
build.gradle
Outdated
compile 'org.jetbrains.kotlinx:kotlinx-serialization-json:1.1.0-RC' | ||
//implementation 'org.jetbrains.kotlin:kotlin-reflect:1.5.0' |
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.
Nitpick but should probably remove dead code.
} | ||
} | ||
|
||
/*suspend inline fun <reified V : Any> get(name: String): V? { |
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.
Should probably remove dead code.
""".trimIndent() | ||
//Adding event listener was causing the client to send a Client2ServerMessage with a null data field. This caused an error | ||
//We make the client return true to avoid that issue. | ||
//Then on the server we only invoke our callback on eventObjects, by checking that payload is a JsonObject. |
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.
Might want to add a TODO to find a way to avoid the initial ignored message.
setJson(name, value) | ||
} | ||
|
||
fun setJson(key: String, value: JsonElement) { |
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.
should we just merge this into fun set(name: String, value: JsonElement)
since they do the same thing?
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.
Yes, we should probably merge them.
Also, I'm now realizing we decided to write the class this way, when we were still abstracting away JsonElements and stuff from end users. We could have fun set(key: String, value: JsonElement)
, and just have end users convert their data to JsonPrimitive(or object or element), instead of us doing it for them. That'd remove a lot of duplication in this class, and in CookieReceiver
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 think those convenience functions for JsonPrimitives are useful, they do lead to some duplication but I think its ok in this case.
} | ||
} | ||
|
||
/*suspend inline fun <reified V : Any> get(name: String): V? { |
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.
Should probably remove dead code.
This pr allows for de/serialization of Server2ClientMessage with Kotlin Serialization. You've seen some of this code before in the previous pr that was closed, but it was harder to find due to so many redundant commits. This should be easier to follow.
Also, I've added a TODO in misc.kt, because while I think Client2ServerMessage should be straightforward to move over, misc.kt has Any.toJson(), and I think that might be a little more involved to fix.
We could do something like the primitiveToJson() function I added to that file.
Objects that are marked @serializable would probably also work if we rewrote it to use Kotlin Serialization.
Ideally, we'd be able to do something like
when (this) {
is serializable -> Json.encodeToString(this)
else -> error("You tried to encode a non serializable class")
}
But, I did a quick search, and I couldn't find a way to check if a class is Serializable, programmatically.
Alternatively, I thought instead of rewriting this Any.toJson(), we could replace usages of it, with something like primitiveToJson()
So, I looked for project usages of this function and randomly selected this one, from StorageReceiver.kt
operator fun set(name: String, value: Any) {
setString(name, value.toJson())
}
It seems like limiting that function to primitive types might work ok. But, we might want to support non primitive types for other function calls.. I haven't gone through every usage of Any.toJson() yet to see if using only primitives would cause problems. It's probably better to support objects too. I just don't see a great way to do that yet.