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

Idea: Allow EffectRouter to be passed to Mobius.loop directly #114

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JensAyton
Copy link
Contributor

The asConnectable at the end of an EffectRouter setup is an annoying abstraction leak. Supporting an array of multiple update types and multiple effectHandler types is pretty unattractive, though.

Here’s a third option: use an underscored protocol to provide a common supertype to EffectRouter<Effect, Event> and Connectable where Input == Effect, Output == Event. I’m not convinced myself, the extra associated types are a pain, but I thought it was worth looking at.

@jeppes

@JensAyton
Copy link
Contributor Author

This also allows effect routers to be written without explicitly naming the effect and event types, although I admit it ends up looking a bit weird:

loop = Mobius.loop(
    update: update,
    effectHandler: EffectRouter()
        .routeEffects(equalTo: .testEffect).to(testEffectHandler)
)
    .start(from: 0)

@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #114 into master will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage    92.3%   92.32%   +0.02%     
==========================================
  Files          46       46              
  Lines        1442     1446       +4     
==========================================
+ Hits         1331     1335       +4     
  Misses        111      111
Flag Coverage Δ
#ios 92.32% <88.88%> (+0.02%) ⬆️
#macspm 92.32% <88.88%> (+0.02%) ⬆️
Impacted Files Coverage Δ
...obiusCore/Source/EffectHandlers/EffectRouter.swift 98.07% <100%> (+0.07%) ⬆️
MobiusCore/Source/Connectable.swift 100% <100%> (ø) ⬆️
MobiusCore/Source/Mobius.swift 92.39% <80%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a68b568...18d4208. Read the comment docs.

@rastersize
Copy link
Contributor

I very much like this idea! The asConnectable thing has bothered me ever so slightly 😸

Copy link
Contributor

@jeppes jeppes left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've been wanting to make this change too.

A few things about the implementation though - I think it would be simpler to just add an overload for the loop builder constructor that takes an EffectRouter instead. That way we can avoid the _EffectHandlerConvertible type and _asEffectHandlerConnectable() can be replaced with a call to the existing asConnectable

@@ -72,6 +72,12 @@ public struct EffectRouter<Effect, Event> {
}
}

extension EffectRouter: _EffectHandlerConvertible {
public func _asEffectHandlerConnectable() -> AnyConnectable<Effect, Event> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the _asEffectHandlerConnectable() methods be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; this is necessary since protocols can’t have multiple levels of access control. In order to publicly conform to the protocol the method must be implemented publicly.

@JensAyton
Copy link
Contributor Author

@jeppes Do you expect we’ll need the Connectable version in the long term, or is it transitional?

@jeppes
Copy link
Contributor

jeppes commented Feb 19, 2020

@JensAyton I think we either need to keep Connectable around or introduce exit-effects to the loop

@kmcbride
Copy link
Collaborator

kmcbride commented Feb 19, 2021

Sorry to bump an old thread, but was something like this ruled out?

extension EffectRouter: Connectable {
    public func connect(_ consumer: @escaping Consumer<Event>) -> Connection<Effect> {
        return compose(routes: routes).connect(consumer)
    }
}

It seems like it would "just work" and avoid the need for any other API changes.

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.

5 participants