[K/JS] Add exportability for Kotlin/JS with conversion to `AsyncItera… - #4625
[K/JS] Add exportability for Kotlin/JS with conversion to `AsyncItera…#4625JSMonk wants to merge 5 commits into
Conversation
|
Important note: it could work only starting 2.3.20 (since |
dkhalanskyjb
left a comment
There was a problem hiding this comment.
We cannot accept this PR as is. We already have several integrations with other asynchronous systems (for example, everything in the reactive/ directory), and this PR is inconsistent with that approach.
- Most notably, we usually don't have
Flowimplement any interfaces (even if it could have)—instead, we provide converter functions, so that the concerns are clearly separated. fromfunctions are questionable: onlyfrom(JsAsyncIterable)is in line with the coldFlowsemantics. In any scenario, it's more consistent forfromto be a extension function onJsAsyncIterableinstead.- Also, none of the new code is tested.
If you would like to work on getting this PR to be merged, please take a look at the existing reactive integrations for inspiration and try mimicking their API. Alternatively, please file an issue instead, and we'll look into implementing the integration ourselves, provided there's a demand for it.
|
@dkhalanskyjb the functions are not designed to be used from Kotlin, but from the TypeScript side (as a part of the @JsExport functionality):
|
|
|
Philosophically, the primary issue with the pull request is that, as proposed, the collection of With this mismatch, it's simply incorrect to have the async iterator API as readily available as This means that some wrapping ceremony is mandatory here, at least to signify the travel between the two worlds. There is a conceptual chasm between The most idiomatic way to implement the conversion, one that's consistent with the other reactive integrations, is via extension functions, like fun <T> Flow<T>.asAsyncIterable(): JsAsyncIterable<T>
fun <T> JsAsyncIterable<T>.asFlow(): Flow<T>We do this for integration with the Java entities as well: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-jdk9/kotlinx.coroutines.jdk9/ (see The syntax from the Java side is indeed unpleasant, as you mention: you have to write something like Another way would be to introduce a wrapper class: public class FlowIterable<out T>(public val flow: Flow<T>): JsAsyncIterableThis can be called with the same amount of ceremony from Kotlin and JS. It's not as nice as extension functions, as we typically short-circuit chains like |
Exactly the same situation we already have with the Flow conversion to AsyncIteratorProtocol in SwiftExport, and we already convert them in this way, so to not break consistency across platforms - it doesn't make sense to introduce manual convertors. The core idea behind our work on interop (both JS and Swift) is to make the use of Kotlin declarations on the target platform idiomatic for that platform (not for Kotlin, in our case, JS). Unfortunately, there is no primitive for push-based async iteration in JavaScript, so the only way to represent Flow in an idiomatic JS way is AsyncIterable.
public class FlowIterable<out T>(public val flow: Flow<T>): JsAsyncIterable
It requires the user to explicitly call those wrappers (which can't be done from the common code, since the wrapper class would exist only for JS/WasmJS targets), which means tons of expect/actual declarations in the sake of (to be honest, I haven't gotten the point in the sake of what). Just one more time to highlight the point: the introduced API (at least right now) is not for use inside of Kotlin (I can make it clear by adding a hidden deprecation), but TypeScript. P.S. I think there could be a need to pass Flow to an imported JavaScript API that expects AsyncIterator/AsyncIterable, so in this case, we definitely will need to introduce a super type of |
Thank you, I hadn't realized this. This is probably a problem as well. Could you provide any links to this functionality (code, documentation, KEEPs)? I couldn't find any mentions of this implicit transformation or how it's performed.
Sorry, this point is unclear. If the whole goal is to use
If the functions intended for
It's for the sake of not breaking the basic contracts. From https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-flow/ (emphasis mine):
Providing a way to collect a Of course, we are also the authors of the documentation and could change it, but having to add "(except when it's being collected on JS using the native async iterable syntax)" throughout the text suggests the abstractions are broken and that the proposed semantics isn't a good fit. With JS, we are at least lucky enough to catch this early, before it reaches the users—I don't know what the situation with Swift is. |
The code is placed here (all the .kt, .swift, .h files)
Since I want to have a common logic exported to all the platforms. Imagine a project with native UI on platforms (SwiftUI, React, as an example). I have a logic placed in
Could you please demonstrate how usage of the AsyncIterator from JavaScript breaks the basic contract? |
Thanks, I'll read it some time later.
And by that, do you mean that you want to take a Kotlin module, export its bindings to another language without writing any extra glue code in Kotlin, and use it as is? This aspiration fails to work even for Java, to which Kotlin is the closest. Not just in terms of extension functions, which are used quite often, but also in
Yes, of course. flow {
println("Emitting 10")
emit(10)
println("Done processing 10")
delay(100)
println("Emitting 2")
emit(2)
println("Done processing 2")
}.collect {
println("Repeat $it times")
repeat(it) {
println("Processing $it...")
delay(100)
}
println("Done!")
}The The async iterator in a This got me thinking: it looks at a glance like |
Thank you so much; With this example, is there any possibility to repeat the same semantic by using Flow on JavaScript (without
Unfortunately, no. This is not my idea; it's just what people ask us. I would be happy to collaborate to make it happen (to make Flow exportable to JavaScript in terms of JavaScript API)
Let's move forward without spending too much time on this. The SwiftExport team and mine have been working on it for a few years, and we've already gotten good results (at least from user feedback). |
|
Hi! The |
|
As discussed with @dkhalanskyjb, the MR was re-worked based on the channel exportability and explicit method converting Firstly, the #4632 should be merged, since the |
53a62d6 to
d7be4b1
Compare
d7be4b1 to
128a528
Compare
…erator`
With the following commit, the `Flow` interface could be consumed on the JavaScript / TypeScript side as follows:
```typescript
for await (const value of FLOW_PRODUCER.asAsyncIterable()) { ... }
```
As well as created from either `AsyncIterable`, `AsyncIterator` or an async generator. With an async iterator the API looks like the following:
```typescript
import { Flow } from "coroutines"
Flow.fromAsyncGenerator(async function* () {
yield await fetch(...)
yield "Hello, World"
});
```
^KT-80733 Fixed
236dbe0 to
1229ff8
Compare
dkhalanskyjb
left a comment
There was a problem hiding this comment.
The build doesn't pass. Updating the ABI dump should fix that.
| public actual suspend fun collect(collector: FlowCollector<T>) | ||
|
|
||
|
|
||
| public fun asAsyncIterable(): JsAsyncIterable<T> = |
There was a problem hiding this comment.
This public function is missing a KDoc. In my opinion, in terms of describing the behaviors, it should be sufficient to specify that it's a shorthand for buffer(0).produceIn(GlobalScope) (that is, its implementation). A usage example and an explanation of when one needs to use this would be appropriate, too.
Also, this function should be either marked as @ExperimentalCoroutinesApi or hidden from Kotlin code. There are too many unclear aspects to it, and there is a potential for its behavior to change, so we can't call this a stable API.
| // so from both Kotlin and TypeScript side it could be used without importing Flow (like in `flowOf` or `flow`) | ||
| // However, the described way of exporting factory functions forces the functions always to be exported (even if people don't use them and don't export Flow), | ||
| // and that may cause bundle size problems (at least right now). | ||
| // So, until the bundle size problem is solved, we keep those factory functions inside Flow, with possibility to move them outside later. |
There was a problem hiding this comment.
Likewise, since we know in advance those functions may need to get moved somewhere else, they need to be marked as experimental.
| import kotlinx.coroutines.flow.internal.* | ||
| import kotlin.coroutines.* | ||
|
|
||
| /** |
There was a problem hiding this comment.
actual definitions of Flow shouldn't include copies of the KDocs already present in common.
| } | ||
| } finally { |
There was a problem hiding this comment.
Why not cancel the async iterator if emit threw an exception?
Note: in this scenario, it is fine to treat CancellationExceptions as a normal return, because that's consistent with the enormous design problem affecting the entirety of kotlinx.coroutines: #3658 We will need to fix the issue in the entire library later.
There was a problem hiding this comment.
But it cancels the iterator, isn't it?
if (!completed) {
iterator.asDynamic().`return`().unsafeCast<Promise<*>>().await()
}There was a problem hiding this comment.
Or do you mean to provide the exception thrown by emit to the iterator via the throw method?
| } | ||
|
|
||
| @Test | ||
| fun testFlowToAsyncIteratorThrow() = runTest { |
There was a problem hiding this comment.
Please check that the flow receives the exception. For example (note: I haven't tried running this, mistakes are likely):
val deferred = CompletableDeferred<Throwable>()
val flow = flow {
emit(1)
try {
emit(2)
} catch (e: Throwable) {
deferred.complete(e)
}
}
// ...
// after failing the flow
assertSame(error, deferred.await())There was a problem hiding this comment.
The same question is here. We've discussed that throwing an error inside of the for await shouldn't change the state of Flow/Channel. And the suggestion is actually to check that it's changed. I feel that we should test the opposite, shouldn't we?
There was a problem hiding this comment.
We've discussed that throwing an error inside of the for await shouldn't change the state of
Flow/Channel.
Yes, and my position is still the same: the state of Flow/Channel should change: #4632 (comment) My point is that such behavior aligns with the intuition and expectations of JS programmers, and we have no reason to diverge from that.
I see now that during the review, I missed the commit that made return and throw only affect the channel, and because of that, I merged the version that doesn't change the state of Channel. It's my mistake, I'm sorry about that.
When it comes to Flow, there's no space for debate, the flow has to observe the exception. The reasoning here goes beyond the JS programmer expectations and concerns the core of what a Flow is. There are two types of Flow:
- Cold flows, and
- Hot flows.
Cold flow collection is semantically a self-contained computation, launched for each collector independently and cleaned up when a collector is no longer interested in receiving values.
flow {
try {
emit(1)
} finally {
delay(100.milliseconds)
println("Done")
}
}.first()
// will print `Done` and only return after thatWhat you're proposing is to drop the computation once the result is no longer needed—which leaks closeable resources and leaves the cleanup computations in finally block unrun. This isn't aligned with Flow semantics and needs to be fixed.
Hot flow collection is shared among collectors—but collectors fail independently. SharedFlow doesn't notice any failures in collectors, so it's perfectly safe to cancel collectors indiscriminately. Only the coroutine scope in which the collection is launched can react to failures.
There was a problem hiding this comment.
Fixed the channel behavior in a separate PR: #4718 Again, my apologies for merging the previous version without noticing that the channel was no longer getting cancelled.
With that change, the proper cleanup for cold flows in this PR should happen automatically, so we'll be able to keep the straightforward buffer(0).produceIn implementation instead of conjuring up something completely separate.
There was a problem hiding this comment.
Sorry, but I completely disagree with the behavior and statements.
-
It's un-debaggable (aka painful) default behavior. It will take hours for Kotlin users to find the
breakplace in JavaScript where theirFlow/Channelwas canceled. -
It's not "the intuition and expectations of JS programmers". There is no contract on the
AsyncIterablelevel that it should be "closed" after itsAsyncIteratorbreak/return/throwing (only the iterator itself). Different AsyncIterables behave differently. ReadableStream is closing itself on break (but to protect users, it also guarantees privileged access to itself, so only one listener at a time can be), while EventEmmitter doesn't close itself, and all the other listeners continue getting new events. -
We're on the same ground that the AsyncIterable should preserve the
Flowcontracts. And I definitely want to provide users with the control over such a behavior (for bothFlowandChannel). My proposal is to have different implementations depending on theFlow, so that if it's the cold one, to cancel it onbreak/return/throw(shall we also provide the same guarantees for the privileged access as ReadableStream? I feel we could, but not sure to do it in this iteration), for the "hot" flow, to keep the original flow as is without cancelling it. -
Regarding
Channel, it's a question, because I don't know the contract (at least the only thing I see that early break on Kotlin side iterations doesn't cancel theReceiveChannel, it works only in the opposite way: if the channel is closed, thenextmethod will throw theClosedReceiveChannelException). However, I still believe that the least painful behavior for theChannelis the contract, thatReceiveChannelcan be canceled only by thecancelmethod (which, btw, is also available from the TypeScript) and not implicitly by the earlyreturn/break/throw.
Also, there is another argument for keeping theChannelbehavior as it is now: with thecancelmethod available and the current behavior on early return, users have more control and can choose whether to usecancelor not (ReceiveChannel).
Changing the behavior will remove this control (so users will not be able to stop listening to the channel without canceling it). -
From my side, I feel that the discussion is already too long in time, and we definitely will not cover all the user expectations and cases with the discussion, so let we annotate all of the new methods with the experimental annotations (so opt-in is required, keeping the room for changing the behavior in the future after user feedback) and release at least some version to receive actual user feedback.
There was a problem hiding this comment.
- Ah, got it - this behaves as expected with the particular implementation. The
CancellationExceptionis raised only when entering the secondfor await. - For
Flow, it also looks good from my side that this aligns withcollectsemantics. - I’m still concerned about the default channel closing on
breakwithout providing privileged access to it.
Also, I realizedasAsyncIterablefeels unnecessary forFlow, since it creates a newChannelper call and offers no meaningful configuration surface there. ForChannel, however, such configuration does make sense, but currently there’s no dedicated API to control that behavior explicitly. - For usability, I’d like to ask again that we consider:
- making
FlowdirectlyAsyncIterableby default (no wrapper call when there is nothing to configure, e.g. no scope/cancellation options), and - adding an explicit conversion/configuration function for
Channel, where cancellation behavior is actually configurable (similar toReadableStreambehavior options).
- making
There was a problem hiding this comment.
So, my proposal here is the following:
- In Channel, add a method similar to
valuesin ReadableStream with the option to not cancel the channel (and delegate creating of the iterator to the method with the cancellation option astrue) - Inside Flow iterator mimics the API of Channel: default iterator which delegates to the default
Channeliterator, andvaluesthat delegate to the channel values.
I'm proposing this specific API to follow the same idea as the:
So the API and control over the semantics would be familiar to the JS devs
There was a problem hiding this comment.
In general—deal, let's do that. Let's figure out how to deal with the remaining edge cases.
Flow
The reason we decided to go for an explicit function for Flow was not exactly that we wanted to parameterize it, but rather, that there's a conceptual mismatch between normal Flow collection and the equivalent to buffer(0).produceIn(GlobalScope).
- Flow executes synchronously, and all cleanup is done by the time
collectreturns. Example: https://pl.kotl.in/y8pdr-kXf - Flow executes in the caller's coroutine scope, not in
GlobalScope.
- can be worked around by configuring the desired context directly in
.flowOn, but 1) is unavoidable with this simple implementation.
Counter-proposal
We could write a custom AsyncIterable implementation for Flow that deals with the asynchronous cleanup, without delegating to Channel. Since return and break return promises, we can wait until collect finishes before resolving them with either the result or an error. This deals with the major remaining mismatch and makes Flow collection from JS seamless.
Channel
Having a separate configurable method sounds okay. I'm not sure about its exact API, though.
- If we're going to expose it to Kotlin, it should look Kotlin-esque, which means something like
fun ReceiveChannel<T>.asAsyncIterable(cancelOnEarlyExit: Boolean): AsyncIterable<T>(probably with some default value forcancelOnEarlyExit). - However, the
ReadableStreamAPI is nothing like that, at all. Itsvaluesaccepts an object with a singlepreventCancelfield. Maybe that's familiar to JS users (and should be implemented for them as the target audience), butclass ChannelAsyncIteratorOptions(val cancelOnEarlyExit: Boolean)is extremely non-conventional.
That is to say, either we hide these conversions from Kotlin (which could be okay), or we make two APIs, or the JS developers will have to deal with the Kotlin-esque API.
There was a problem hiding this comment.
I've got it for the Channel and will prototype soon.
Regarding Flow. Would we want to have the version without cleaning (the same as the one in the Channel with preventCancel parameter)? If so, does it make sense just to use buffer(0).produceIn(GlobalScope). for the method?
There was a problem hiding this comment.
No, there's no reason not to await a Flow cleanup. The cleanup will run anyway, we're only controlling whether it can run concurrently with the code that comes after for await.
| assertNextStepToBe(iterator, 1, done = false) | ||
| assertNextStepToBe(iterator, 2, done = false) | ||
| val returnResult = iterator.`return`(42).await() | ||
| assertEquals(true, returnResult.done) |
There was a problem hiding this comment.
Shouldn't returnResult.value be 42? Why not check that?
| assertNextStepToBe(iterator, 1, done = false) | ||
| // Call throw() to cancel the iterator | ||
| val error = js("new Error('test error')") | ||
| assertFailsWith<Throwable> { iterator.`throw`(error).await() } |
There was a problem hiding this comment.
Please also test the no-argument throw.
| val flow = flow { | ||
| emit(1) | ||
| emit(2) | ||
| emit(3) |
There was a problem hiding this comment.
We need to verify that finally cleanups at the point of cancellation run (and in fact, that cancellation does happen).
There was a problem hiding this comment.
Should it happen? As far as I remember, we've discussed that throwing an error inside of for await should have no impact on either the Channel state or the Flow state.
There was a problem hiding this comment.
Replied in the comment thread above. In short: yes, it's mandatory for a correct implementation of Flow collection.
| flow.take(2).collect { results.add(it) } | ||
| assertEquals(listOf(1, 2), results) | ||
| yield() // Allow cleanup to happen | ||
| assertTrue(returnCalled, "return() should be called on cancellation") |
There was a problem hiding this comment.
We also need to check that 3, 4, and 5 weren't requested from asyncIterator (as opposed to just getting dropped somewhere on the way). To that end, onReturn could have the type (Int) -> Unit, where Int is the index of the last requested element.
1bef704 to
196f906
Compare
…tor`
With the following commit the
Flowinterface could be consumed on the JavaScript / TypeScript side as follows:As well as created from from either
AsyncIterable,AsyncIteratoror an async generator. With an async iterator the API looks like the following:^KT-80733 Fixed