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

Add unsafeRunSync* syntax for JS Dispatcher #2846

Merged

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Feb 27, 2022

Following up on #2835 (comment), replays the convenience methods added to IO in #2613 for Dispatcher (JS-only).

It needs to be added as syntax in core due to the dependency on SyncIO for the unsafeRunSync execution.

Daniel proposed an alternative in #2835 (comment):

We basically have to weigh whether a syntax enrichment in core is better or worse than a significantly-altered signature in std (e.g. taking a function of type G[A] => A as a parameter).

IIUC it's proposing something like this on Dispatcher:

def unsafeRunSyncToFuture[G[_]: Sync, A](
  fa: F[A],
  limit: Int,
  unsafeRunSync: G[A] => Either[Throwable, A]
)(implicit F: Async[F]): Future[A] = {
  unsafeRunSync(F.syncStep[G, A](fa, limit)) match {
    case Left(t) => Future.failed(t)
    case Right(Left(fa)) => unsafeToFuture(fa)
    case Right(Right(a)) => Future.successful(a)
  }
}

which I would then call like this:

dispatcher.unsafeRunSyncToFuture(fa, 32, (_: SyncIO[A]).attempt.unsafeRunSync())

But at that point, I think I could just as easily write:

fa.syncStep[SyncIO](32).unsafeRunSync().fold(
  dispatcher.unsafeRunAndForget(_),
  _ => () // whatever
)

This assumes I don't care about the result ... which in many cases I don't, because I'm completing a Deferred or offering to a Queue.

The syntax proposed here gets us this:

dispatcher.unsafeRunSyncToFuture(fa, 32)

So, subjective obviously. And not necessarily mutually exclusive.

I also wish there was a better story for the limit parameter, which we can't fish out of the IORuntime in this case. So it will almost definitely be hardcoded, which requires some iota of thinking or tempts a lazy Int.MaxValue which is probably fine for the complete/offer use-cases.

@djspiewak
Copy link
Member

Btw haven't forgotten about this. I'm thinking we might be able to make it a bit more convenient in some cases. Will see if I can find time to experiment a bit.

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Did some experimentation. I think this is about the best we can do for now. We could remove the syncLimit parameter for any F: LiftIO, but that would actually be really weird because then you would have code that would compile until you add a constraint, at which point scalac would start telling you that you're missing a parameter. So that seems worse than just having it behave this way all the time.

@djspiewak djspiewak merged commit 0abb94d into typelevel:series/3.x Mar 17, 2022
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