-
-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add async iterable Stream
class that inherits from Signal
#462
Conversation
Stream
class that inherits from Signal
and is an async iteratorStream
class that inherits from Signal
cc: @jasongrout @vidartf |
Cool. If I understand it correctly, you're long-polling the signal? |
I wouldn't have used the term long polling, but maybe it is apt, because there is always a |
Thinking a bit more about event life cycle, could you try the following scenario: A signal listener, emit the same signal that triggers it (only the first time it get triggers to avoid infinite recursion). When are the iterator/iterable displayed?
My guess is that the iterator/iterable will not be displayed between the two executions of the listener as they are asynchronous. If this is the case, we should warn user of potential inconsistency when using that feature as the state that triggered a promise resolution may not exist any more. |
@fcollonval You're absolutely right. There is a problem here. I'm going to put this PR back into draft mode to think about it. |
Stream
class that inherits from Signal
Stream
class that inherits from Signal
@fcollonval I was thinking about it realized that maybe The other thing I did was to make sure you never lose a promise by having the internal Thanks for finding it. It hurt my head for a while 🥲 |
How does the async iterator approach interact with cleanup compared to the connect/disconnect pattern? Previously, the signal emitter would keep a reference to the slot, but with the stream approach it gets reversed, right? How will this affect resource cleanup? I haven't thought it through yet, so I'm hoping you have 😀 I.e. I can imagine this would improve the situation where listeners aren't being cleaned up since there are still references to them, but will it instead prevent emitters from being cleaned up since there are listeners who have references to them? Note: I might be mixing up how JS resource management works these days. |
@vidartf I think this will improve the situation, as you say, because when you use a |
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.
Thanks @afshin
@afshin Right, so let's say we have widgets A and B. B listens to a stream on A via the new API. Then the owner of widget A figures out that it is no longer needed, and disposes of it ( |
I think you're exposing the need for a |
@vidartf I added a |
This PR adds a
Stream
class thatextends Signal
and implementsIStream
, which means that in addition to being a signal, it is also an async iterable. It can be used as a drop-in replacement for anySignal
(orISignal
) in an application if the author wishes to expose an async iterable API to complement signal connection/disconnection.This PR also exposes a
protected blocked: number
property of aSignal
so that sub-classes can implement correct blocking behavior.Examples
Subscribe to a stream via
connect
/disconnect
:Subscribe to a stream as an async iterable:
Generate an async iterator over the stream.