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

POC: Redesign without IO #156

Closed
wants to merge 2 commits into from
Closed

POC: Redesign without IO #156

wants to merge 2 commits into from

Conversation

gatorcse
Copy link
Contributor

@gatorcse gatorcse commented Sep 7, 2018

I experimented with changing #155 to not require IO. The stated reasons for specifying IO were for Ref and Deferred, but those only require Sync and Concurrent respectively. I wanted to see if we could remove the requirement for IO, allowing Monix or other implementations to be used.

This is just a proof of concept. I didn't update the tests or anything, it also adds a F[_] parameter all over the place. This can probably be wrapped up in an abstract class or something to simplify the API. I just wanted to see if it would work.

@gatorcse gatorcse self-assigned this Sep 7, 2018
cache: DataSourceCache = InMemoryCache.empty
def run[F[_]: Sync: Par, A](
fa: Fetch[F, A],
cache: DataSourceCache[F] = InMemoryCache.empty[F]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't compile because the call to .empty requires the implicit Sync instance, which is in the implicit argument list that follows, but that is not important to the concept.

env: Option[Ref[IO, Env]]
private def performRun[F[_]: Sync: Par, A](
fa: Fetch[F, A],
cache: Ref[F, DataSourceCache[F]],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't need IO, just an implicit Sync

for {
deferred <- Deferred[IO, FetchStatus]
deferred <- Deferred[F, FetchStatus]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't need IO, just an implicit Concurrent

@purrgrammer
Copy link
Contributor

Interesting, thanks for the PR. I'm going to update it to match what's in redesign right now and make it compile and I'll come back here to write my thoughts on this approach.

@raulraja
Copy link
Contributor

raulraja commented Sep 9, 2018

@gatorcse great stuff 👏 . The clear advantage of being more generic is that we can run Fetch to monix Task and any other data type that already provides instances for cats-effect.

@purrgrammer
Copy link
Contributor

I managed to make it compile, removing the F parameter from a couple places like DataSource (it is instead now passed to DataSource#fetch and DataSource#batch). I still need to update the examples/tests and the documentation, but I think I'm going to incorporate this into the redesign branch.

Awesome work @gatorcse, thanks for doing the heavy lifting with this changes.

@purrgrammer purrgrammer mentioned this pull request Sep 11, 2018
13 tasks
@purrgrammer
Copy link
Contributor

Added these improvements to #155

@fedefernandez fedefernandez deleted the non-io-poc branch October 16, 2023 07:39
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.

None yet

3 participants