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

KTOR-7584 HTMX extension for Ktor #4553

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

KTOR-7584 HTMX extension for Ktor #4553

wants to merge 21 commits into from

Conversation

bjhham
Copy link
Contributor

@bjhham bjhham commented Dec 16, 2024

Subsystem
Server, New plugin

Motivation
KTOR-7584 First-class HTMX support

Solution
This extends the routing and HTML builder DSL with some handy HTMX helpers. I tried to ere on the conservative side here by not including everything and adding an experimental flag so we can change things later after testing. We don't have the HTML fragments extension yet in the HTML DSL, but you can still do everything you need using valid HTML and attributes.

osipxd and others added 21 commits December 12, 2024 11:02
* Use stdlib `use` extension-function
* Update type checks
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
* Support multiple client engines for JS/WasmJs
* CIO client Js/WasmJs support
* Enable CIO tests in client tests
* Make client engines `data object` to have `toString`
* Make ClientLoader work with multiple engines on js/wasmJs and so test CIO engine on nodejs(js+wasmJs)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Test libcurl build with conan

* Take both curl and openssl from the same build

* test no linux cache

* Move K/N linux cache disabling to just `ktor-client-curl` module
Some services use 403 instead of 401. Changing them might be impossible. With this change Ktor can flexibly work with any broken service.

---------

Co-authored-by: Osip Fatkullin <[email protected]>
* KTOR-7679 Allow disabling body decoding on server
…EmbeddedServer.stop (#4481)

* Add startSuspend/stopSuspend in EmbeddedServer
* Make EngineTestBase work on js/wasmJs
@bjhham bjhham requested review from osipxd and Mr3zee December 16, 2024 13:34
Copy link
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

Finally, it is here :)

Comment on lines +20 to +22
header(HxRequestHeaders.Request, "true") {
configuration()
}
Copy link
Member

Choose a reason for hiding this comment

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

Probably this header should be applied to the route passed in HxRoute constructor:

public value class HxRoute(route: Route) : Route by route.header(HxRequestHeaders.Request, "true") {

@ExperimentalHtmxApi
@KtorDsl
@JvmInline
public value class HXRoute(private val route: Route) : Route by route {
Copy link
Member

Choose a reason for hiding this comment

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

Minor

Suggested change
public value class HXRoute(private val route: Route) : Route by route {
public value class HxRoute(private val route: Route) : Route by route {

client.get("htmx") {
headers[HxRequestHeaders.Request] = "true"
headers[HxRequestHeaders.Trigger] = "#button"
}.bodyAsText().trim()
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add negative tests?


@ExperimentalHtmxApi
@HtmlTagMarker
public class HxAttributes(override val map: DelegatingMap) : StringMapDelegate {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in a shared module?


public object HxAttributeKeys {
/**
* issues a GET to the specified URL
Copy link
Member

Choose a reason for hiding this comment

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

Here and in some other files KDocs are starting with a lowercase letter. Could you fix it?
I also think that we could add a link to the official docs to the KDoc of this class: https://htmx.org/reference/#attributes-additional

public var location: String? by HxResponseHeaders.Location
public var pushUrl: String? by HxResponseHeaders.PushUrl
public var redirect: String? by HxResponseHeaders.Redirect
public var refresh: String? by HxResponseHeaders.Refresh // TODO boolean
Copy link
Member

Choose a reason for hiding this comment

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

Should we make it boolean now? Or ExperimentalHtmxApi allows breaking changes? 😅

Comment on lines +34 to +39
public val on: On
get() = On(map)

public fun on(event: String, script: String) {
map["hx-on:$event"] = script
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember what we decided here. Keep both options of syntax?

on["event] = "script"
on("event", "script")

public var pushUrl: String? by HxAttributeKeys.PushUrl
public var select: String? by HxAttributeKeys.Select
public var selectOob: String? by HxAttributeKeys.SelectOob
public var swap: String? by HxAttributeKeys.Swap
Copy link
Member

Choose a reason for hiding this comment

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

We've had type-safe implementations for some of these fields. Will it be part of some future PR?

@osipxd osipxd force-pushed the 3.1.0-eap branch 2 times, most recently from 7c22c8a to f555d1a Compare December 19, 2024 10:14
@osipxd osipxd force-pushed the 3.1.0-eap branch 3 times, most recently from 3481339 to 5090504 Compare January 9, 2025 13:11
@osipxd osipxd force-pushed the 3.1.0-eap branch 2 times, most recently from 2c3b4d8 to 4a59355 Compare January 9, 2025 14:59
Base automatically changed from 3.1.0-eap to main January 9, 2025 15:50
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.

9 participants