-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Conversation
* Use stdlib `use` extension-function * Update type checks
Co-authored-by: Osip Fatkullin <[email protected]>
…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
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.
Finally, it is here :)
header(HxRequestHeaders.Request, "true") { | ||
configuration() | ||
} |
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.
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 { |
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.
Minor
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() |
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.
Could you also add negative tests?
|
||
@ExperimentalHtmxApi | ||
@HtmlTagMarker | ||
public class HxAttributes(override val map: DelegatingMap) : StringMapDelegate { |
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 this be in a shared module?
|
||
public object HxAttributeKeys { | ||
/** | ||
* issues a GET to the specified URL |
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.
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 |
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 make it boolean now? Or ExperimentalHtmxApi
allows breaking changes? 😅
public val on: On | ||
get() = On(map) | ||
|
||
public fun on(event: String, script: String) { | ||
map["hx-on:$event"] = script | ||
} |
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 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 |
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.
We've had type-safe implementations for some of these fields. Will it be part of some future PR?
7c22c8a
to
f555d1a
Compare
3481339
to
5090504
Compare
2c3b4d8
to
4a59355
Compare
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.