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

[WIP] exposing RedisEnvironment via environment #739

Closed
wants to merge 1 commit into from

Conversation

m-kalai
Copy link
Contributor

@m-kalai m-kalai commented Jan 29, 2023

  • another approach to pushing codec and executor dependency from RedisCommand creation to it's execution

Copy link
Member

@mijicd mijicd left a comment

Choose a reason for hiding this comment

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

A few minor comments from my side, but we have a bigger problem, which is that RedisEnvironment (or BinaryCodec with RedisExecutor) leaks in user-land, thus going against the service pattern and the changes you did in #727.

Comment on lines +38 to +39
lazy val layer: ULayer[Redis] =
ZLayer.succeed(Redis)
Copy link
Member

Choose a reason for hiding this comment

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

Well, you may as well remove the RedisLive object and pack everything in Redis, e.g.:

object Redis {
  lazy val layer: ULayer[Redis] = ZLayer.succeed(new Redis)
}

In the future we may consider renaming it to live, perhaps it'll be more consistent with the ecosystem.

Comment on lines +9 to +14
lazy val layer = ZLayer {
for {
codec <- ZIO.service[BinaryCodec]
executor <- ZIO.service[RedisExecutor]
} yield new RedisEnvironment(codec, executor)
}
Copy link
Member

Choose a reason for hiding this comment

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

Super nitpick:

Suggested change
lazy val layer = ZLayer {
for {
codec <- ZIO.service[BinaryCodec]
executor <- ZIO.service[RedisExecutor]
} yield new RedisEnvironment(codec, executor)
}
lazy val layer =
ZLayer {
for {
codec <- ZIO.service[BinaryCodec]
executor <- ZIO.service[RedisExecutor]
} yield new RedisEnvironment(codec, executor)
}

def executor: RedisExecutor
final case class RedisEnvironment(codec: BinaryCodec, executor: RedisExecutor)

private[redis] object RedisEnvironment {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this should be private at all now that it's a part of all commands. In fact, I'm thinking if it would be wise to replace it with BinaryCodec with RedisExecutor.

@m-kalai
Copy link
Contributor Author

m-kalai commented Jan 30, 2023

A few minor comments from my side, but we have a bigger problem, which is that RedisEnvironment (or BinaryCodec with RedisExecutor) leaks in user-land, thus going against the service pattern and the changes you did in #727.

yes you're right, i was just thinking that maybe it's ok to expose codec and executor to user. saw some libs doing that, but yes it doesn't play well with service pattern.

to follow service pattern exactly, we should move implementation of commands from trait to some class and pass codec and executor as constructor params. if we don't want to mix codec and executor into Redis trait, or expose them to the user via environment, or depend on use of implicits, there is probably no other way. or we can just leave it as is.

@m-kalai m-kalai closed this Jan 31, 2023
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.

2 participants