Skip to content
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

async generators: for await ... of !this #192

Open
malcolmstill opened this issue Apr 13, 2021 · 5 comments
Open

async generators: for await ... of !this #192

malcolmstill opened this issue Apr 13, 2021 · 5 comments

Comments

@malcolmstill
Copy link

Does it make sense in an async function* (async generator) to for await (const _ of <x>) where x is something other than this?

What I'm playing around with is turning a websocket connection into an async generator as per https://www.bignerdranch.com/blog/asyncing-feeling-about-javascript-generators/

Then doing:

export async function* WebSocketStream({ socket }) {
  let messages = []

  yield <p>Awaiting data</p>

  for await (const msg of socket) {
    messages.push(msg)
    yield (
      <div>
        {messages.map((m, i) => (
          <div key={i}>{m}</div>
        ))}
      </div>
    )
  }
}

where socket is a websocket (from new WebSocket(addr))

Right enough this works, and I get a stream of messages. But I have some unexpected behaviour where I see certain changes only when data comes across the websocket. This leads me to think that the issues are because I am no longer awaiting on this.

  1. Does it make sense to not for await this?
  2. Is there some other way I should be doing this? Intuitively I want to for await the combination (i.e. a multiplex) of this and socket, but I'm not sure how to do that (funnily enough I came across this answer of yours @brainkim: https://stackoverflow.com/a/58809794 whilst googling)
@Raiondesu
Copy link

Raiondesu commented Apr 14, 2021

There is an example in docs addressing precisely this. In the example, the createInterval function is completely Crank-agnostic, just like the websocket is.

So, I guess, it does make sense to not always use this, as iterating over it is simply a way to detect prop updates.

EDIT: As for seeing unexpected behaviour - it is not quite obvious from the issue what exactly is unexpected. Maybe a playground link would help here. :)

P.S. Maybe making something like this.propUpdates iterable instead of this would be more readable and less confusing, but the current way is definitely more "cranky" IMHO. 😅

@brainkim
Copy link
Member

brainkim commented Apr 14, 2021

@malcolmstill

Right enough this works, and I get a stream of messages. But I have some unexpected behaviour where I see certain changes only when data comes across the websocket. This leads me to think that the issues are because I am no longer awaiting on this.

Can you speak more about what you’re seeing and what you expect?

Does it make sense in an async function* (async generator) to for await (const _ of ) where x is something other than this?

I would say it could make sense with a couple caveats. Firstly, if you’re looping over an async iterator in a component that is not the context, you risk causing a deadlock. When a component unmounts, we call the return() method on the generator. This is sort of like passing a return statement into the generator at the next yield operation so we break out of the loop and cause the source websocket iterator to return as well. However, in Crank, async generator components are continuously resumed, so you’re never likely to see an async generator component suspended at a yield. Rather, you’ll most likely end up suspended at the top of the for await loop, waiting for another message. This is mostly okay, except this means that the async generator component will not resume until the next message is received, because we’re stuck at an await or for await and not a yield. This might that your websocket iterator remains open for longer than you expect, causing the actual underlying connection to remain open as well. If you don’t receive any additional data from the websocket iterator, then that’s a deadlock, because the component will just be suspended indefinitely.

A second problem might be that the websocket async iterator closes early, in which case you wouldn’t render any more values because the component function has returned. In Crank, once a generator component returns, it sticks to its last returned value, so you’d have to manually unmount and remount the component to render something new. If you have an automatically reconnecting websocket async iterator, you might not have to worry about this, but it’s something to think about.

Lastly, Crank is already on the frontier, but async iterators themselves are another frontier as well. So a lot of this stuff is yet to be explored, and there is the potential that it’s all just wasted time and energy. Personally, I would say we still don’t have high quality utilities to express what you’d want, where you pull from both the context async iterator and your websocket async iterator in tandem. You may want to check out my sibling project repeater.js if you haven’t already.

I’ve been meaning to update repeaters for a while now but you know how it goes, and recently I’ve been thinking about what sort of breaking changes I should make with regard to the project, because apparently it’s picked up in usage thanks to NFTs or something? I’ve been talking myself off the ledge with regard to some of the breaking changes I wanted to make, and the project is relatively stable.

In any case, one potential function from repeaters you could look into is Repeater.latest(), which takes multiple async iterators and returns the latest values for each. You could theoretically pass both the websocket and the component context into this function and work through both, yielding elements based on both messages and props. However, it’s likely that you also might need a different kind of combinator like flatMap() or switchMap(), so that the component props “feed” into the creation of a new websocket connection somehow. These combinator functions are something I‘ve been meaning to implement for a while now, I just haven’t had the time.

Again, while the idea of reactive programming with async iterators is tempting, the question I want to ask you is, is it any better than using sync generator components and raw websocket callbacks? Especially because you’re diving into a new framework and all, maybe it might be better to try doing things this way first. Async generators are tricky to reason about because of the high level of concurrency and unfamiliarity of both yielding and awaiting in the same component, so I typically try to steer people to trying sync generator components first.

I hope this answers your questions. Let me know your thoughts and thank you for opening this issue!

@Raiondesu

P.S. Maybe making something like this.propUpdates iterable instead of this would be more readable and less confusing, but the current way is definitely more "cranky" IMHO. 😅

Yes, this has been suggested multiple times, but I think there’s value in not having iteration be a separate method, insofar as these loops are sort of fundamental to the component lifecycle, and, for instance, trying to pull multiple values without yielding anything is a hard runtime error. Something to think about though!

@malcolmstill
Copy link
Author

Thank you both for your replies

@Raiondesu thank you, I hadn't found that particular example

EDIT: As for seeing unexpected behaviour - it is not quite obvious from the issue what exactly is unexpected. Maybe a playground link would help here. :)

Can you speak more about what you’re seeing and what you expect?

I've distilled it down to:
https://codesandbox.io/s/crankjs-hvmz6

Though I've replaced the websocket with the createInterval function (I've called it pretendWebsocket) you pointed to @Raiondesu.

And here is the issue I was seeing: using only for await (const of socket) reorders and drags are "synchronised" to pretendWebsocket yielding, i.e. I drag the window, it doesn't actually move, and then on receiving a bit of data on the socket the window finally jumps to where it was dragged.

@brainkim with your suggestion of looking at repeater, I tried what you can see in the above link. You can see I can drag and reorder the windows continuously if using for await (const msg of Repeater.merge([this, socket])).

(with my full example / actual websocket, I seem to be able to continuously drag windows but the window raising is synchronised to the websocket, not sure why that differs to the codesandbox example)

Hopefully that explains what I'm trying to do / the issue I was seeing?

Again, while the idea of reactive programming with async iterators is tempting, the question I want to ask you is, is it any better than using sync generator components and raw websocket callbacks? Especially because you’re diving into a new framework and all, maybe it might be better to try doing things this way first. Async generators are tricky to reason about because of the high level of concurrency and unfamiliarity of both yielding and awaiting in the same component, so I typically try to steer people to trying sync generator components first.

I am not convinced at all it is any better 😄 . It was, as you say, really tempting (and felt quite natural) just to think of the UI and the data coming off of the websocket as just an asynchronous stream...in other words it simply fit my own mental model.

Indeed, in my actual code I am sharing the websocket amongst a bunch of windows, and having written some basic filtering generators, filter out different messages into different windows. There may well be issues I haven't come across yet doing this, but the code came out so simple and fit my mental model so well it just feels like there's something there.

@malcolmstill
Copy link
Author

In fact, here's an example using a filter: https://codesandbox.io/s/crankjs-filter-3sbih

@Raiondesu
Copy link

Hmm, this whole example gives me a feeling that if one wants to program UI using async iterators in Crank, usage of Repeater.merge() is necessary to achieve any intuitive behaviour.

Maybe this alone makes repeaterjs/repeater#67 pt.1 worth doing after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants