Skip to content

{Audio,Video}Stream: use AsyncIterableIterator#272

Merged
nbsp merged 4 commits intomainfrom
nbsp/audiostream/iterator
Sep 23, 2024
Merged

{Audio,Video}Stream: use AsyncIterableIterator#272
nbsp merged 4 commits intomainfrom
nbsp/audiostream/iterator

Conversation

@nbsp
Copy link
Copy Markdown

@nbsp nbsp commented Sep 19, 2024

instead of having a typed EventEmitter

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 19, 2024

🦋 Changeset detected

Latest commit: aeb27e1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@livekit/rtc-node Minor
@livekit/rtc-node-darwin-arm64 Minor
@livekit/rtc-node-darwin-x64 Minor
@livekit/rtc-node-linux-arm64-gnu Minor
@livekit/rtc-node-linux-x64-gnu Minor
@livekit/rtc-node-win32-x64-msvc Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nbsp nbsp force-pushed the nbsp/audiostream/iterator branch 2 times, most recently from 513dbff to aadbf24 Compare September 19, 2024 19:05
@nbsp nbsp requested review from bcherry and theomonnom September 20, 2024 03:58
Base automatically changed from nbsp/bump/ffi-v0.10.2 to main September 20, 2024 06:37
An error occurred while trying to automatically change base from nbsp/bump/ffi-v0.10.2 to main September 20, 2024 06:37
aoife cassidy added 2 commits September 19, 2024 23:49
@nbsp nbsp force-pushed the nbsp/audiostream/iterator branch from 0daf003 to 8667110 Compare September 20, 2024 06:50
return { done: true, value: undefined };
}
}
return new Promise((resolve) => (this.queueResolve = resolve));
Copy link
Copy Markdown
Contributor

@lukasIO lukasIO Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this.queueResolve might get overridden before being called?
so the async iterator could get stuck

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Mutex helper that Théo has mentioned already might make sense here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could it? i don't think that's the case, it only gets overridden if you call next() twice without awaiting the first one, which you shouldn't do anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which you shouldn't do anyway.

agreed that it shouldn't be done, but seems like a footgun regardless?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to return a copy of the same promise if next is called twice?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I think best case scenario is a promise chain, which is exactly what the Mutex class does.

@nbsp nbsp requested a review from lukasIO September 23, 2024 19:22
@nbsp nbsp force-pushed the nbsp/audiostream/iterator branch from 6315094 to 32cf737 Compare September 23, 2024 19:23
@theomonnom
Copy link
Copy Markdown
Member

Can you do the same for VideoStream?

@nbsp nbsp changed the title make AudioStream use AsyncIterableIterator {Audio,Video}Stream: use AsyncIterableIterator Sep 23, 2024
@nbsp nbsp merged commit 1f1a075 into main Sep 23, 2024
@nbsp nbsp deleted the nbsp/audiostream/iterator branch September 23, 2024 19:49
@github-actions github-actions Bot mentioned this pull request Sep 23, 2024
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

Successfully merging this pull request may close these issues.

3 participants