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

switch to object options for Stream.async apis #3213

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Conversation

tim-smart
Copy link
Member

@tim-smart tim-smart commented Jul 10, 2024

Up for discussion :)

Copy link

changeset-bot bot commented Jul 10, 2024

🦋 Changeset detected

Latest commit: cb0720c

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

This PR includes changesets to release 31 packages
Name Type
effect Major
@effect/cli Major
@effect/cluster-browser Major
@effect/cluster-node Major
@effect/cluster-workflow Major
@effect/cluster Major
@effect/experimental Major
@effect/opentelemetry Major
@effect/platform-browser Major
@effect/platform-bun Major
@effect/platform-node-shared Major
@effect/platform-node Major
@effect/platform Major
@effect/printer-ansi Major
@effect/printer Major
@effect/rpc-http Major
@effect/rpc Major
@effect/schema Major
@effect/sql-d1 Major
@effect/sql-drizzle Major
@effect/sql-kysely Major
@effect/sql-mssql Major
@effect/sql-mysql2 Major
@effect/sql-pg Major
@effect/sql-sqlite-bun Major
@effect/sql-sqlite-node Major
@effect/sql-sqlite-react-native Major
@effect/sql-sqlite-wasm Major
@effect/sql Major
@effect/typeclass Major
@effect/vitest Major

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

@dilame
Copy link
Contributor

dilame commented Jul 10, 2024

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 Queue by my own – i understand that this Queue is used under the hood, which grants me extra knowledge about how my code works.

Plus if we will have a new options for Queue in the future – we will not need to re-implement all the methods that just passes props from one place to another. I can't think of any new options for Queue now, but just in case.

Don't see any reason to hide such implementation details from the user.

@IMax153
Copy link
Member

IMax153 commented Jul 10, 2024

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. unbounded, dropping, etc.) is a better UX than having to pass the buffer to use. Furthermore, for this case specifically the Queue constructors are effectful, and while that's not an issue since the effect can be resolved internally, it obfuscates things further.

From my perspective, I like the refined API proposed by @tim-smart.

This also closes #3200.

@mikearnaldi
Copy link
Member

Up for discussion :)

Dropping elements by default looks like a very bad choice. Bounded by default is kind of the only thing that make sense

@IMax153
Copy link
Member

IMax153 commented Jul 10, 2024

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.

@dilame
Copy link
Contributor

dilame commented Jul 10, 2024

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 Queue by himself, so no need to describe things, everything is described in the documentation
https://effect.website/docs/guides/concurrency/queue

@IMax153
Copy link
Member

IMax153 commented Jul 10, 2024

Or just let user to pass Queue by himself

Again, this is not a pattern we use anywhere else in the library and I'm not sure it's a desirable one.

@mikearnaldi
Copy link
Member

Also note that passing Queue.unbounded() is not "passing a queue" but rather passing a queue constructor. Per se as an additional option it isn't necessarily bad

@dilame
Copy link
Contributor

dilame commented Jul 10, 2024

Right now we are discussing the necessity of share operator, and seems like there will be the first place where this pattern will be used

https://discord.com/channels/795981131316985866/1258936229219532851

@tim-smart
Copy link
Member Author

Or just let user to pass Queue by himself, so no need to describe things, everything is described in the documentation https://effect.website/docs/guides/concurrency/queue

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.

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

Successfully merging this pull request may close these issues.

From Discord: Memory Leak in Effect: zipLatest with async Stream Causing Fiber Overflow
4 participants