Replace java.time with kotlin.time in websockets configuration #4315
Conversation
| * Ping interval or `null` to disable pinger. Please note that pongs will be handled despite this setting. | ||
| */ | ||
| @Deprecated("Use variant with kotlin.time.Duration", level = DeprecationLevel.ERROR) | ||
| public inline var DefaultWebSocketServerSession.pingInterval: Duration? |
There was a problem hiding this comment.
Now I'm not sure that it worth to keep these extensions with deprecation as they could confuse users 🤔
There was a problem hiding this comment.
Yeah, I think it would be a little confusing. It would be nice to drop the JVM source path entirely for this plugin too. It's an easy migration for them anyway.
| * Ping interval meaning pinger is disabled. | ||
| * @see pingIntervalMillis | ||
| */ | ||
| public const val PINGER_DISABLED: Long = 0 |
There was a problem hiding this comment.
To be discussed: With this constant code seems a bit clearer, but I'm not sure if I chose the right place for it.
There was a problem hiding this comment.
I think it'd be best under the WebSockets companion since that's the only type users are generally exposed to.
There was a problem hiding this comment.
The only thing I don't like is that it will be two constants, one for a client plugin and another for a server plugin.
I thought about making these constants pointing to the same constant in the shared module, but it will result in imports ambiguity.
public const val PINGER_DISABLED: Long = DefaultWebSocketSession.PINGER_DISABLEDThere was a problem hiding this comment.
@bjhham, what do you think?
There was a problem hiding this comment.
Oh I see, maybe in that case it can be a top-level declaration.
osipxd
force-pushed
the
osipxd/websocket-kotlin-time
branch
from
fb70368
to
f381e4f
Compare
| * Ping interval or `null` to disable pinger. Please note that pongs will be handled despite this setting. | ||
| */ | ||
| @Deprecated("Use variant with kotlin.time.Duration", level = DeprecationLevel.ERROR) | ||
| public inline var DefaultWebSocketServerSession.pingInterval: Duration? |
There was a problem hiding this comment.
Yeah, I think it would be a little confusing. It would be nice to drop the JVM source path entirely for this plugin too. It's an easy migration for them anyway.
| * Ping interval meaning pinger is disabled. | ||
| * @see pingIntervalMillis | ||
| */ | ||
| public const val PINGER_DISABLED: Long = 0 |
There was a problem hiding this comment.
I think it'd be best under the WebSockets companion since that's the only type users are generally exposed to.
osipxd
force-pushed
the
osipxd/websocket-kotlin-time
branch
3 times, most recently
from
e7426af
to
7217fc8
Compare
Introduce PINGER_DISABLED constant
osipxd
force-pushed
the
osipxd/websocket-kotlin-time
branch
from
7217fc8
to
a7e604c
Compare
Subsystem
Client/Server,
ktor-server-websockets,ktor-client-core,ktor-shared:ktor-websocketsMotivation
KTOR-7446 Replace
java.timeoptions in Websockets pluginSolution
Replaced
java.time.Durationwithkotlin.time.Duration. MovedDurationextensions to the common source set. AddedDurationsupport to a websocket client.