-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
switch to object options for Stream.async apis #3213
Conversation
🦋 Changeset detectedLatest commit: cb0720c The changes in this PR will be included in the next version bump. This PR includes changesets to release 31 packages
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 |
836180f
to
927ed52
Compare
3827ac3
to
8e96dd8
Compare
As usual, i would vote for the composable API. Stream.async(
(emit) => {
//...
},
{ buffer: Queue.unbounded() },
); I believe that the main point of having a single-responsibility primitives is having the ability to compose them. In addition, this way we don't hide the internal details from the user – if i provided a Plus if we will have a new options for Don't see any reason to hide such implementation details from the user. |
We do not provide such an API anywhere else within Effect, so I disagree that this should not be hidden from the user. Given the underlying complexity in stream, I think giving users the flexibility to choose the buffer strategy (i.e. From my perspective, I like the refined API proposed by @tim-smart. This also closes #3200. |
Dropping elements by default looks like a very bad choice. Bounded by default is kind of the only thing that make sense |
I'm fine with leaving it bounded by default I guess but we should probably add a note to the JSDoc that explains the semantics of each option. |
Or just let user to pass |
Again, this is not a pattern we use anywhere else in the library and I'm not sure it's a desirable one. |
Also note that passing |
Right now we are discussing the necessity of https://discord.com/channels/795981131316985866/1258936229219532851 |
The issue with passing a constructor is that it doesn't guarantee exclusive use by the stream, and it surfaces implementation detail. While unlikely, we may want to swap the usage of Queue with another buffer implementation in the future. |
927ed52
to
9c8cde8
Compare
8e96dd8
to
71005fc
Compare
9c8cde8
to
438c486
Compare
71005fc
to
d783de8
Compare
438c486
to
560d6aa
Compare
d783de8
to
9944e90
Compare
560d6aa
to
73c5a64
Compare
9944e90
to
42578ff
Compare
73c5a64
to
e7e388f
Compare
42578ff
to
3c03281
Compare
e7e388f
to
66bd9f8
Compare
3c03281
to
70da220
Compare
66bd9f8
to
b160287
Compare
Up for discussion :)