ReactKit / SwiftState

Elegant state machine for Swift.
MIT License
904 stars 93 forks source link

suggestion on transition and condition, which should include Event #34

Closed frogcjn closed 8 years ago

frogcjn commented 9 years ago

There is a case that the event has associate values which cannot be enumerated all the possible values, so that it is not possible to add all Event Route into State Machine.

In this case, the only way to do it is to use condition.

But unfortunately, condition only includes transition which has no information about the event.

So I suggest to add Event information into transition.

frogcjn commented 9 years ago

For example: .LoadAction(Int) Event .CancelAction Event

.Pending State .Loading(Int) State

.LoadAction event has an associate integer to suggest which task it wants to trigger. .Loading state has an associate integer to suggest which task it is loading.

when the .LoadAction triggered, and the machine on .Loading state with the same associate number, then the Event should be ignored.

inamiy commented 9 years ago

Hi, I added your above example as a XCTestCase in https://github.com/ReactKit/SwiftState/commit/cc6bc8f77473f58705e3c0f936346af4f2f10054. Is this what you are looking for?

frogcjn commented 9 years ago

Yes.

Because transition has no information on event, we should add all possible events to the machine.

        for actionId in 1...100 {
            machine.addRouteEvent(.LoadAction(actionId), transitions: [ .AnyState => .Loading(actionId) ], condition: { $0.fromState != .Loading(actionId) })
        }

if (1) I want the Int as greater as I want, or (2) the associate value is a generic type from subclass's enum, or (3) the associate value is a data

then the StateMachine cannot work on this kind of situations. These situations have the same character: event's associate value cannot be fully enumerated by current class.

inamiy commented 9 years ago

Oh, OK I got it! You are right, and current typealias Condition = (transition: Transition) -> Bool has too few parameters to distinguish Event and other stuffs e.g. userInfo.

I think it should be something like:

typealias ConditionContext = (event: Event, transition: Transition, userInfo: Any?)
typealias Condition = ConditionContext -> Bool

And for this enhancement, we will need a major version bump. Let me take some time to work on this :wink:

frogcjn commented 9 years ago

Yes! Thanks!

frogcjn commented 9 years ago

And another suggestion is, can you use Optional.None to represent the meaning of AnyState or AnyEvent? Because it needs code to add an nil init for an event, and it is ugly to add one more State though the fact the machine hasn't that State.

If there is a place needs nil to represent for the meaning of "AnyState" or "AnyEvent", just put "Event?" or "State?" as the parameter's type.

inamiy commented 9 years ago

Thanks for above feedback as well! This will also be another big breaking change, so I will refactor existing codes for both ideas :)

frogcjn commented 9 years ago

Thanks:)

inamiy commented 8 years ago

Sorry for late reply. I just finished writing a rough draft for solving your above issues in https://github.com/ReactKit/SwiftState/pull/36.

Please check and tell me your thoughts :wink:

inamiy commented 8 years ago

This issue has been completed in #36 and ver 4.0.0. Please see Ver 4.0.0 Release Notes for more detail.

@frogcjn Thanks again for your contribution! :tada: