-
Notifications
You must be signed in to change notification settings - Fork 30k
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
lib: make diagnostics_channel async iterable #35532
Conversation
Review requested:
|
826c870
to
6548fcb
Compare
cc @nodejs/diagnostics Now that diagnostics_channel has landed, I'd like to get a fresh look at this to decide if async iterable support is something we want. No docs currently, but I can write some if people think this is a feature worth having. |
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.
diagnostics_channel
is implemented fully synchronously, right? If we add async iterablility to it, we'll be adding asynchronocity to the API, which will cause confusion to users (as the timing from the main API and the async API will be wildly different). Unless I'm misunderstanding it, I'm opposed to adding async iterability to this API (it's not an asynchronous API).
Fair point. I didn't implement it as a requirement, so one could use the regular API when sync guarantees are needed, but then that'd need to be documented well and could be a source of confusion. 😕 |
This is just an idea for a possibly helpful extension to the diagnostics_channel feature in #34895. Not sure how valuable this is, so I've pulled it out to a separate PR where people can 👍 or 👎 if they think it's a worthwhile addition.
Depends on: #34895
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes