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

Remove unnecessary State constraint for reusability #27

Closed
wants to merge 3 commits into from

Conversation

inamiy
Copy link

@inamiy inamiy commented Jun 28, 2018

This is just an example code to improve framework's type signatures.
(Note: Test fixes are not included)

  • State should not be constrained by any protocols (especially with associated type). Otherwise, it won't be reusable and composable when making a larger state.
  • Same can be said for Action which should be isolated from State.
  • Only Mutation should be the protocol that associates both State and Action
  • extension Actions where Action == CounterAction is more intuitive to make an extension compared to extension Actions where State == CounterState, where CounterAction is implicitly associated inside CounterState.

@ra1028
Copy link
Owner

ra1028 commented Jul 9, 2018

thx @inamiy !
This idea that became the basis of VueFlux was close to that.
But, VueFlux has a global event bus system that affects all States with the same type like Store<State>.actions.anyAction().
Your idea makes it difficult to make sense of it.
I also have concern that it'll be ambiguous to constrain State as a reference-type.
Due to some other concerns, VueFlux has become a type signature such as now.

However, Your idea is practically correct.
Let me think about it.

@inamiy
Copy link
Author

inamiy commented Jul 9, 2018

I also have concern that it'll be ambiguous to constrain State as a reference-type.

Oh, I wasn't aware of State only allowing reference type 👀
But we can easily overcome it by changing to open class Store<State: AnyObject, Action> (updated in 9b9cbcf ),
since only Store requires this precondition but no other classes really need it.

(For example, in general, Dispatcher is not responsible of filtering out its dispatch mechanism to reference-only instances.)

@inamiy
Copy link
Author

inamiy commented Jul 9, 2018

2eaf9e7 Changed Dispatcher-extracting-key from State.Type (implicit) to Dispatcher.self (more explicit)

@ra1028
Copy link
Owner

ra1028 commented Jul 9, 2018

@inamiy
Thx!
It looks good to me 👍

I still have the following concerns.
Do you think about?

- Practically correct is the `Store<Mutations>`?  But, this is not makes sense...
- The length of the code of global event bus becomes too long.
- Is not it `Mutations` instead of `Store` that we needs to constrain `State` as reference-type?

@inamiy
Copy link
Author

inamiy commented Jul 10, 2018

  • Is not it Mutations instead of Store that we needs to constrain State as reference-type?

Mutations with associatedtype State: AnyObject is not (essentially) needed.
If you add it, Store<State, Action> will not compile, and 9b9cbcf will be needed anyway.

Minimum type constraint is always the winner of code abstraction and reusability.

  • Practically correct is the Store<Mutations>? But, this is not makes sense...

As you mentioned, that will be even more counter-intuitive because Mutations now hides associated State.
In general, Store should be the container of generic State just like Array that is generic over its Element.

  • The length of the code of global event bus becomes too long.

True, but moving Action from associated type to generic type parameter leads to more generalization (better parametricity), since it is (should be) a fundamental, independent type.
If too long type name is annoying, typealias will be helpful.

@ra1028
Copy link
Owner

ra1028 commented Jul 10, 2018

I think that your idea is roughly correct, but I would like to discuss a little more.

Minimum type constraint is always the winner of code abstraction and reusability.

You are correct.
In terms of essence, reference-type constraints are not required anywhere in VueFlux.
However, in actually, Mutations only really make sense with a reference-type because it does not support inout reference.
e.g

struct FooState {
    var prop: Int  = 0
}

struct FooMutations: Mutations {
    func commit(action: FooAction, state: FooState) {
        var state = state

        switch action {
        case .increment:
            state.prop += 1
        }
    }
}

Since this is standard behavior of Swift, it's acceptable for me, I also think that it might be confusing to users.

In general, Store should be the container of generic State just like Array that is generic over its Element.

Agreed 👍
I think it's true.

True, but moving Action from associated type to generic type parameter leads to more generalization (better parametricity), since it is (should be) a fundamental, independent type.
If too long type name is annoying, typealias will be helpful.

hmm
I think the too long type name isn't related to the correctness of the type-signature, but I think it is a big problem in readability.
typealias is effective for this case, but increase the types leads to an increase the complexity of the application.
I would like to think about another I/F if possible.

@ra1028 ra1028 self-requested a review July 10, 2018 07:53
@inamiy
Copy link
Author

inamiy commented Jul 10, 2018

Mutations only really make sense with a reference-type because it does not support inout reference.

Yes, it makes sense whenever Mutations work together with Store.
In such case, Store.State is AnyObject-only so that Mutations.State will automatically follow this constraint as well.

But in essence (as you said), Mutations doesn't always have to work together with Store.
For example, someone might want to use this framework for his new library and reuse Mutations but not Store.
In such (very special) case, having Mutations.State: AnyObject or Mutations.State: VueFlux.State constraint will be a heavy burden for him.

To wrap up, this pull request is just my own perspective of sticking to the simplest types as possible, but I also understand that other users may look this as less consistency because of it's abstractness.
I won't go deeper into this topic for now, so please take another moment of thinking for further improvements 😉

@inamiy inamiy closed this Jul 10, 2018
@ra1028 ra1028 reopened this Jul 10, 2018
@ra1028
Copy link
Owner

ra1028 commented Jul 10, 2018

I want to advance thinking after merging this PR.
will merge this later 🙏

@inamiy inamiy closed this Oct 20, 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.

None yet

2 participants