Add kotlinx serialization extension methods for HttpFetcher

[dead8309]

No description provided.

[bitspittle]

The API looks great!

One thing I am now realizing is something is still missing: in addition to window.http.*, we also have window.api.*, which is basically like window.http but designed for use with server API routes. (In fact, ApiFetcher was first but I added HttpFetcher later since it was more generally applicable)

So here's the question...

Do we create a single serialization module, where covers both HttpFetcher and ApiFetcher extensions can live together? Or do we have a separate module for each one?

I'm actually leaning to using one module for both, because I think in practice it's unlikely for HttpFetcher and its extension methods ever to get upstreamed to JetBrains, so instead we can just create a single module which is simple and convenient for Kobweb users (and we can always break it up later if we need to).

But if we do that, it demands a different name. Maybe just kobwebx-core-serialization or something like that? (Since ApiFetcher lives in kobweb-core)

The two choices that I'm seeing:

• One module, which encompasses both HttpFetcher and ApiFetcher, but it needs a different name at that point.
• Two modules, one for browser, and the other for core. And the core module has an api dependency on the browser module. We'd still need a new name for the ApiFetcher one.

Any opinions?

[dead8309]

I agree, I'm also leaning towards going with a single module approach. It will simplify things for Kobweb users and as you mentioned, we can always split it later if we need to.

For the internal structure, we can organize the HttpFetcher and ApiFetcher related code into separate packages within the module.

Do you have any thoughts on how we should proceed with implementing this? Should I start by outlining the internal structure of the new module?

As for the name of the module maybe one of kobwebx-core-serialization ,kobwebx-serialization-extensions. It's upto you

[dead8309]

Sorry! Skipped the Latest branch part in Contributing file

[bitspittle]

For the internal structure, we can organize the HttpFetcher and ApiFetcher related code into separate packages within the module.

Do you have any thoughts on how we should proceed with implementing this? Should I start by outlining the internal structure of the new module?

I'll take a look at your commits in just a sec, but what you outlined in the first sentence -- one module with multiple packages in it -- sounds good to me.

As for the name of the module maybe one of kobwebx-core-serialization, kobwebx-serialization-extensions.

I'm currently leaning towards kobwebx-core-serialization-kotlinx (as there are other serialization libraries out there besides the kotlinx one). But that is a mouthful, so I will think a bit more about it. Feel free to let me know if you think it is awful :)

Still, you don't need to feel blocked by a name. We can always check it in with one thing and we still have time to come up with another name before releasing 0.19.3.

(Not something to worry about in this commit, but I will also be considering if maybe we should merge the Kobweb worker serialization extension utilities into whatever we're creating here)

[bitspittle]

Everything generally looks like it is in the right place, so this should be the only round of comments. I don't think anything should be too difficult to resolve!

[bitspittle]

Instead of exposing this method, let's keep it private and use the following pattern in the extension code:

suspend inline fun <reified T> ApiFetcher.get(
    apiPath: String,
    headers: Map<String, Any>? = null,
    abortController: AbortController? = null,
    autoPrefix: Boolean = true,
    serializer: DeserializationStrategy<T> = serializer()
): T = Json.decodeFromString(serializer, window.api.get(apiPath, headers, abortController, autoPrefix).decodeToString())

[dead8309]

But window.api.get don't accept autoPrefix. I think we dont need Json.decodeFromString as we are calling the http methods from HttpFetcherSerializationExt.kt which will handle json decoding

We want to prepend api calls with /api so i made that method public. Maybe i can add a private method in ApiFetcherExtensions file for that ?

[bitspittle]

The code snippet I included above is copied from my local copy of the code and it seems to be working fine? (autoPrefix is indeed an argument in ApiFetcher.get)

[dead8309]

Sorry!! I was reading that as window.http not window.api. making the change

[bitspittle]

BTW, LoP brought up a great question to me offline, and this is something I can do this in a followup commit (so not blocking your changes), but I thought I'd reach out to you first in case you had any thoughts about it.

Basically: should we treat body payloads as something that can also be serialized?

This would look like this:

suspend inline fun <reified B, reified R> ApiFetcher.post(
    apiPath: String,
    headers: Map<String, Any>? = null,
    body: B? = null,
    abortController: AbortController? = null,
    autoPrefix: Boolean = true,
    bodySerializer: SerializationStrategy<B> = serializer(),
    responseSerializer: DeserializationStrategy<R> = serializer()
): R = window.http.post(
    toResource(apiPath, autoPrefix),
    headers,
    body?.let { Json.encodeToString(bodySerializer, it).encodeToByteArray() },
    abortController,
    responseSerializer
)

Any thoughts?

[dead8309]

Definitely!!

[dead8309]

We only want to support body serialization for API routes, right? Or do you think I should also add support for HTTP requests as well?

[bitspittle]

We only want to support body serialization for API routes, right? Or do you think I should also add support for HTTP requests as well?

Ah yeah we should do both. The APIs are definitely related and should feel consistent.

[bitspittle]

I'm going to go ahead and approve these changes. We'll merge after HttpFetcher also has its body APIs updated.

If you're busy and would like me to handle that in a followup commit, just let me know.

Thanks again for this PR!