Skip to content
This repository was archived by the owner on Mar 11, 2020. It is now read-only.

update fs2 to 1.0.0-M1 - #61

Closed
wedens wants to merge 2 commits into
Verizon:masterfrom
wedens:dependencies-update
Closed

wedens wants to merge 2 commits into
Verizon:masterfrom
wedens:dependencies-update

Conversation

@wedens

@wedens wedens commented Jul 26, 2018

Copy link
Copy Markdown

Most changes are s/Effect/ConcurrentEffect/g. Also ExecutionContext is now required only when resolving ConcurrentEffect which makes notificationPool useless.

@timperrett

Copy link
Copy Markdown
Contributor

@adelbertc how does this affect us with nelson? @rossabaker for visibility

@wedens

wedens commented Jul 26, 2018 •

Copy link
Copy Markdown
Author

And please tell me if I've missed some place where specific ExecutionContext was important (blocking or something). It's difficult to spot due to it being implicit.

@rossabaker rossabaker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @wedens!

What made the strengthening of the constraint to ConcurrentEffect necessary? I'm guessing you followed the errors, but I'm not spotting it myself.

This would require eventually upgrading Nelson to cats-effect-1.0 as well. That will require http4s-0.19, and similar work in helm. I think it's important to keep the libraries current anyway, especially as they get toward this LTS release. Nelson can upgrade at its own pace.

@wedens

wedens commented Jul 26, 2018 •

Copy link
Copy Markdown
Author

Concurrent+Effect+cancellation=ConcurrentEffect.

What previously required Effect+ExecutionContext, now requires Concurrent+Effect.

Concurrent is required for Stream#joinUnbounded, signalOf and most other stuff from fs2. Effect is required for Effect#runAsync in loadp (from knobs package object). Any code path that loads a config leads to loadp. Cancellation (currently) is not used in knobs (directly or transitively).

Using F[_]: ConcurrentEffect is a bit more convenient than F[_]: Effect : Concurrent, but I can change to the later, if you think it's worth it.

UPD: I think this runAsync can be replaced with Sync[F].delay(Effect[F].toIO(fa).unsafeRunSync)) if blocking is really necessary here. But it still requires Effect.

@wedens

wedens commented Jul 27, 2018 •

Copy link
Copy Markdown
Author

Also, I suspect Resource#load should be shifting to some blocking ExecutionContext (this PR doesn't change it)

@wedens

wedens commented Nov 13, 2018

Copy link
Copy Markdown
Author

Looks like it's already updated in master. Closing.

@wedens wedens closed this Nov 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants