Make JS iteration over ReceiveChannel cancel iteration on early exits - #4718
Make JS iteration over ReceiveChannel cancel iteration on early exits#4718dkhalanskyjb wants to merge 2 commits into
Conversation
Also, mark `asyncIterator` as `ExperimentalCoroutinesApi`.
dkhalanskyjb
changed the title
…tion over ReceiveChannel
|
@dkhalanskyjb as we discussed in #4625 I've added methods for the |
dkhalanskyjb
left a comment
dkhalanskyjb
left a comment
There was a problem hiding this comment.
I can't request changes for my own pull request, which complicates the review. Please consider making another branch on top of mine and opening a separate PR. No need to bother making it pretty, it's only for code review.
| import kotlin.js.Promise | ||
|
|
||
| @JsImplicitExport(couldBeConvertedToExplicitExport = true) | ||
| public actual interface ReceiveChannel<out E> : JsAsyncIterable<E> { |
There was a problem hiding this comment.
Note that ReceiveChannel is still JsAsyncIterable. We have to choose a single entry point: either the channel itself is iterable, or the users go through a separate function. Having both defeats the purpose of the function to educate the user about the channel cancellation behavior.
| */ | ||
| @ExperimentalCoroutinesApi | ||
| // We can't use DeprecationLevel.HIDDEN, because the generated declaration will also be deprecated in .d.ts | ||
| @LowPriorityInOverloadResolution |
There was a problem hiding this comment.
I don't think this does anything. This overload is never in conflict with the other one.
| @@ -1,5 +1,5 @@ | |||
| @file:OptIn(ExperimentalJsExport::class, ExperimentalStdlibApi::class) | |||
| @file:Suppress("EXPOSED_FUNCTION_RETURN_TYPE", "INVISIBLE_REFERENCE", "EXPOSED_SUPER_INTERFACE") | |||
| @file:Suppress("EXPOSED_FUNCTION_RETURN_TYPE", "INVISIBLE_REFERENCE", "EXPOSED_SUPER_INTERFACE", "EXPOSED_PARAMETER_TYPE") | |||
There was a problem hiding this comment.
ChannelIteratorOptions being internal but visible as an parameter of a public overload is not acceptable. I tried using this from another Kotlin/JS project, and on the Kotlin side, the IDE does not hide the overload that accepts ChannelIteratorOptions, doesn't show that it can't be constructed, and in general, behaves as if the overload is entirely valid. This can lead users on a wild goose chase.
| * - `preventCancel = false` or omitted: early iterator completion cancels the channel. | ||
| */ | ||
| @ExperimentalCoroutinesApi | ||
| // We can't use DeprecationLevel.HIDDEN, because the generated declaration will also be deprecated in .d.ts |
There was a problem hiding this comment.
Alright, does DeprecationLevel.ERROR work?
If not, we'll have to settle for just the boolean overload and remove this one. Getting people relying on autocompletion on the Kotlin side into a trap where they try and fail to construct a ChannelIteratorOptions object would be a major usability issue.
| import kotlin.js.Promise | ||
|
|
||
| @JsImplicitExport(couldBeConvertedToExplicitExport = true) | ||
| public actual interface ReceiveChannel<out E> : JsAsyncIterable<E> { |
There was a problem hiding this comment.
With the new function(s), we need to reevaluate ReceiveChannel implementing JsAsyncIterable directly.
Since both possible AsyncIterable behaviors on exit—cancelling or not cancelling—may surprise a subset of users, and we provide a function controlling this, we'd like to emphasize that there's this variability in behavior and encourage checking the options. If someone just writes for await (const v of channel), nothing here indicates a non-trivial choice being made.
I realize ReadableStream does this as well—it's both iterable and has a values method—but as you pointed out, its default cancellation behavior is more obvious, due to it being exclusive to a single consumer.
| @ExperimentalCoroutinesApi | ||
| // We can't use DeprecationLevel.HIDDEN, because the generated declaration will also be deprecated in .d.ts | ||
| @LowPriorityInOverloadResolution | ||
| @JsName("values") // We use "values" here to mimic the ReadableStream API: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream |
There was a problem hiding this comment.
Is ReadableStream.values an AsyncIterator or an AsyncIterable? I couldn't easily find this in the documentation.
| * @param options iteration behavior options: | ||
| * - `preventCancel = true`: early iterator completion does not cancel the channel; | ||
| * - `preventCancel = false` or omitted: early iterator completion cancels the channel. | ||
| */ |
There was a problem hiding this comment.
Since we don't expect Kotlin users to call this function, we shouldn't generate an API reference page for it, so please /** @suppress */ it.
2 participants
Also, mark
asyncIteratorasExperimentalCoroutinesApi.