nickfargo / state

Composable, heritable first-class states
statejs.org
Other
93 stars 8 forks source link

guards behaving unexpectedly on substates and seemingly not working at all on transitions #17

Open AdrianRossouw opened 10 years ago

AdrianRossouw commented 10 years ago

Ok, i've run into another snag. It was my expectation that the admit / release guards are passed down to substates, unless I specifically choose to override it.

The problem I have here is your typical website authentication workflow. There's a portion of the site you can access once you have authenticated, and there's a portion while anonymous (notably the login page).

I guess my expectation was that :

After some experimenting, I managed to get my required functionality by using a transition from the offline.* to online.* state, and then manually moving the user back to offline if the condition fails. But for the life of me I could not get guards on transitions to even fire, ever. From my understanding, the guards work to choose which of many viable transitions to fire, but it seems I could only ever get it to fire the first one defined.

I'm actually not sure I want to rely on transitions for something this integral, because there can only be one .. the precedence rules are pretty complex, and it just feels like something that could wipe out all authentication checks on a site if another developer adds a transition anywhere else in the tree.

I'm sure you are aware, but in javascript does not guarantee a predictable key order, chrome specifically is(was?) known to behave differently to everything else there.

You really can't even pretend to depend on them in my experience. Most shallow or deep clone implementations reverse the key order, and for the most part you have no control over how the objects come into your functions, so having them just randomly not work isn't feasible.

what's wrong with arrays?

I never really understood why I needed to give a transition a key in the first place. It feels like transitions should be an array of objects with an optional name property.

I also don't see the reason for the different syntax styles (selectors as properties vs selector as key) between the guards and the transitions.

I realize this is a bug, but I actually think the vast majority of use cases for guards don't need a query selector, but also by forcing a key the transition between having a single guard and having multiple involves awkward query string incantations (making a '**' key when you add the second guard feels wrong).

Also, I kind of feel that state guards are something you will need multiple of. in a lot of cases. For me i want to able to slap some admit guards on a state that maps to a page being rendered on a site, that checks that all the dependencies to render that page have been completed.

So I guess something like :

state @,
  offline:
    release: @checkAuthenticated
    login:
      transitions: [{
          name: 'doLogin'
          target: 'online.*'
      }]
  online:
    profile:
      admit: [ @hasProfile, @hasEmail, @isPublic ]

A lot of the problems I seem to run into have to do with the expressiveness and permissibility of the DSL. If you accept any and all input, you're going to end up with things that you didn't anticipate.

issue #10 would go a long way towards narrowing this stuff down. I would add things like state.transition, state.guard etc.

nickfargo commented 10 years ago

It was my expectation that the admit / release guards are passed down to substates, unless I specifically choose to override it.

  • guards operated more on terms of enter/depart,
  • and will be fired whenever the state is made active
  • that would mean on enter/depart and arrive/exit.

Those expectations make a lot of sense.

Presently guards are heritable only via protostates (not superstates or parastates), and they’re pre-evaluated at the endpoints of the transition only, i.e. the depart and arrive sites. (There’s a note about this in the api and literate source, though as yet nothing in the documentation proper that I can see.) However, apart from simplifying the implementation, mostly, there’s no strongly held opinion behind those choices.

It may suffice to implement one or the other of those — that is, either …

… but not both, necessarily; indeed in many ways superstate-heritability and pre-walking would achieve the same effect. I think I’d rather lean toward the latter, but there may be some benefit to expressiveness in pursuing full heritability that I’m not seeing yet.

transitions

You’ve almost certainly caught a regression. Weak test coverage there; I am bad.

Per the present (intended) implementation, transition expressions (when working properly) should be considered unordered. A state’s available transitions are meant to be differentiated according to the transition guards, and so it’s up to the author to avoid/resolve ambiguities, or suffer the nondeterministic consequences.

This goal has math/theoretical roots in the idea of labeled transition systems, where any two states may be connected by several transitions, which are then uniquely differentiated by labels, in whatever form those might take.

what's wrong with arrays?

Likely nothing per se, however, part of the reason for the object-like treatment is that a Transition is, essentially, just a special kind of State, and by that line of reasoning, it made sense to keep the language for expressing transitions as similar as possible to that for expressing states, and ideally, mostly just an extension thereof.

And this leads into the one of the more compelling practical reasons to require that transitions be named, which are consequent from how a transition traverses the state tree:

Now the great irony being, once again, that this isn’t yet implemented properly either. While TransitionExpressions do imply a name since you’re made to hang them off an object’s key, at present the Transitions created from them aren’t actually given a proper corresponding name — and if you take the owner.state().path() while a transition is underway, you can see the nonsense for yourself as it returns something like State.Substate., where there’s this garbage dot . hanging off the end.

So the end goal for this is to have some regular way to express a path that clearly, explicitly indicates an active transition, e.g.:

'aState.aSubstate!aTransition'

or thereabouts. Arrays of transition expression objects that bear a name property could do the trick, and maybe serve as a last-resort order-of-evaluation determination, too, though I fear for the complexity that might arise in the face of heritability from protostates, etc.

A lot of the problems I seem to run into have to do with the expressiveness and permissibility of the DSL. If you accept any and all input, you're going to end up with things that you didn't anticipate.

Yeah, that’s a really good observation. It’s a growing concern, as this little state expression library is over time kind of revealing itself to be approaching a full-fledged DSL. Simplicity, regularity, and appealing to intuition will save the day … and, to wit, advice and issue reports like yours are helping a ton.

nickfargo commented 10 years ago

I also don't see the reason for the different syntax styles (selectors as properties vs selector as key) between the guards and the transitions.

I actually think the vast majority of use cases for guards don't need a query selector, but also by forcing a key the transition between having a single guard and having multiple involves awkward query string incantations (making a '**' key when you add the second guard feels wrong).

The original motivation for how guards are stored and evaluated was merely economy of implementation:

stateInstance._.guards = {
  'guardType': {
    'selector': predicate,
    'selector': predicate,
    // ...
  },
  // ...
}

Simple in theory and internally, but perhaps the API leaves this too exposed to the user, and not sufficiently flexible or intuitive. The data structure may need to be fleshed out further, perhaps toward something where a guardType holds an array of formal GuardExpression objects that define selector and predicate properties … or maybe something still short of that and simpler, if possible.

I kind of feel that state guards are something you will need multiple of. in a lot of cases. For me i want to able to slap some admit guards on a state that maps to a page being rendered on a site, that checks that all the dependencies to render that page have been completed.

So I guess something like :

state @,
  offline:
    release: @checkAuthenticated
    login:
      transitions: [{
          name: 'doLogin'
          target: 'online.*'
      }]
  online:
    profile:
      admit: [ @hasProfile, @hasEmail, @isPublic ]

The lines

release: @checkAuthenticated

admit: [ @hasProfile, @hasEmail, @isPublic ]

make a compelling case.

Those method references may also need to be reducible to literals, such as a string, to preserve the semantics of the state expression in all lexical contexts; e.g. for the case of something like

state Constructor.prototype, 'abstract',
  offline: 'default',
    aMethod: -> yes
    release: @aMethod
  online:
    aMethod: -> no

in which, first, the reference this.aMethod here would not yet exist at “expression-time”, and second, this may render a state expression un-portable, bound to its prevailing function context (which may be acceptable in specific cases but generally is to be avoided). So then, maybe accept instead

state Constructor.prototype, 'abstract',
  offline: 'default',
    aMethod: -> yes
    release: '@aMethod' # or
    release: 'this.aMethod' # both equal to indirection of function literal
    release: -> @aMethod
  online:
    aMethod: -> no

I would add things like state.transition, state.guard etc.

Agreed. I like the direction of these explicit-type-specifier functions, for the benefit of both the state expression interpreter and the human author/reader.

AdrianRossouw commented 10 years ago

guard string format

i was using that as short hand, but generally i'd do -> @method. But let's try to avoid complicating things with more magic strings for now =).

Also, the virtual epistate, prototypical inheritance of states, -thing has never worked right for me, so I never use that form of inheritance (sticking to proto- and sub- states). I wasnt sure what my expectations were at the time though, so I can't comment on that yet.

transition names

As for why the naming of transitions feels odd to me, it's the fact that when you name the states, they become explicit targets that you can change to. There's no time that you go @state('-> transitionName'), and you even mention that they are mostly there because something might be observing the names. The default transition that gets created doesn't really have a name either, so it's obviously not required required.

By giving it a name, i would expect to be able to go @state().transition('authenticate'), to select a transition, especially when i'm not sure it's the calling code's responsibility to know what state it will end up in. This is practically what I am using named custom events to replicate.

guard inheritance

I think all that's needed is to do the pre-transition walk of the tree and fire all the guards.

nickfargo commented 10 years ago

i was using that as short hand, but generally i'd do -> @method. But let's try to avoid complicating things with more magic strings for now =).

Ah ok. It’s true, ad-hoc grammar runs amok fast. May turn out a good idea down the road though, we’ll see.

transition names

It’s a fair point about anonymous transitions; they have their place. Although I do lean toward pushing users to name their transitions when they are declared inside a state expression, as a matter of hygiene (error tracing is a use case that comes to mind), in similar spirit to how it’s good practice to name one’s functions.

i would expect to be able to go @state().transition('authenticate'), to select a transition

Indeed the goal I’ve had in mind there — this is part of the work that’s coming up on the concurrency-regions branch — is to migrate from what is presently

State::change = ( targetState, options ) ->
State::go = State::change

to a pair of user-facing complimentary methods do and go backed by an (ostensibly) private execute method:

State::do = ( transitionExpression, args, options ) ->
  # Determine a `targetState` from the named `transitionExpression`
  # Forward to `execute`

State::go = ( targetState, args, options ) ->
  # Determine a `transitionExpression` from the named `targetState`
  # Forward to `execute`

State::execute = ( transitionExpression, targetState, args, options ) ->

(Elided here is an intermediary schedule method that precedes execute in this flow, where schedule forces transitions into a queue when necessary, from which they are executed transactionally.)

So the go method would otherwise work just like it (and change) does now, while the do method allows the user to execute a transition instead by naming one of a state’s predefined transition expressions.

This sets up, hopefully, a tidy syntactical analogy between doing a named transition from a state and invoking a named method on an object.

object.state().do('oneTransition', ['some', 'args']);
object.state().do('anotherTransition');

guard inheritance

… do the pre-transition walk of the tree and fire all the guards.

Makes sense to me.

I wonder, can you also imagine a need for guards that would still target the endpoints specifically, as they do (only) now?

To put it another way: whereas the proposed behavior for guards tracks with the exit and enter events, rather than with depart and arrive as they do now, might there still be a need for guard types that act only on departure and arrival, as it were?

AdrianRossouw commented 10 years ago

I wonder, can you also imagine a need for guards that would still target the endpoints specifically, as they do (only) now?

I don't actually think so. Conceptually they are much easier to understand as things that stop the state they are defined in from activating/deactivating.

What I am more concerned about is whether the tree being locked off entirely is a sane decision to make as a default behaviour. The thing that my mind keeps on coming back to about this is unix tree permissions. Being able to give chmod +rx /some/deeply/nested/folder

This also brings up why we don't actually look at the return values of the enter/depart events.

I'm also obviously +1 for the targeting of named transitions via do(), it feels like a missing piece.

nickfargo commented 10 years ago

… do the pre-transition walk of the tree and fire all the guards.

What I am more concerned about is whether the tree being locked off entirely is a sane decision to make as a default behaviour.

Right, and as I recall, this was a part of the early thinking for having guards be evaluated at the endpoints (depart arrive) only, as well as for making guards not heritable along the superstate axis. Too powerful by default.

So … does this then make the case for defining guard types that would track all four transitional events (depart exit enter arrive), instead of just the tree-walkers (exit enter)?

Otherwise we’d seem to be juggling a contradiction, are we not … I have that feeling that I’m missing something obvious …

AdrianRossouw commented 10 years ago

Yeah, there is a contradiction here. You'd want to have the ability to do both. Something that occurs to me as well, is that by trying to give these guards different and distinct key names from the events they guard (to support the shorthand syntax), the whole thing just becomes a protracted session of the synonym-game.

I think this is another sign that guards really want to be an array of objects, with query/predicate/event properties to define when they fire. if you want the branch lock-down functionality, you would do something like :

state @,
  online:
    guards: [
        { event:'enter, arrive', allow: false }
    ]
nickfargo commented 10 years ago

Indeed, banging one’s head against a thesaurus … hurts, eventually. In my not inconsiderable experience.

The ambiguity of admit and release mapping to (depart|exit) and (enter|arrive), respectively, is troublesome.

As I cast about for name sets that might map to (depart exit enter arrive), the least-terrible options I can imagine are, in increasing order of terribleness:

  1. (canDepart canExit canEnter canArrive)
  2. (release canExit canEnter admit)

If the simplicity of (release admit) must be lost, the clarity of can* across the board is appealing. (Less attractive alternatives that occur to me include let* and allow*.)

It may also be tolerable to define can* for the “shorthand namespace” of a StateExpression, and map that to just (depart etc.) for definition within the guards category; e.g. such that

expression = state
  enter: -> # event listener
  canEnter: -> # guard predicate

# would be interpreted as equivalent to
expression = state
  events:
    enter: -> # listener
  guards:
    enter: -> # predicate

Of course, it’s important to tread carefully here, as this directly affects the shorthand syntax for state expressions, in ways that can easily (or even will certainly) break user code. Despite this peril, though, I am still partial to reserving a small set of terms in the shorthand namespace, as there is good value in providing reprieve from the chore of always having to structure out the state expression’s components into their proper categories (methods events guards etc.), when there’s a reasonable chance for the interpreter to correctly infer the categorization.

I think this is another sign that guards really want to be an array of objects, with query/predicate/event properties to define when they fire.

Here we have some freedom, I think, to tweak with the brains inside StateExpression, where if it sees

expression = state
  guards: [
    selector: '**'
    events: 'enter, arrive'
    predicate: -> false
  ,
    # a second object
  ,
    # etc...
  ]

this could be transformed — or “indexed”, if you like — into something like

expression = state
  guards:
    enter: [
      selector: '**'
      predicate: -> false
    ]
    arrive: [
      selector: '**'
      predicate: -> false
    ]

or the more formalized (possibly more optimisable for V8 et al)

expression = state
  guards:
    enter: [
      [GuardExpression] { selector: '**', predicate: -> false }
    ]
    arrive: [
      [GuardExpression] { selector: '**', predicate: -> false }
    ]

and maybe instead of arrays we return an associated unique id from addGuard, as with addEvent, with which to removeGuard later if necessary (and mutable):

expression = state
  guards:
    enter:
      'uid1': [GuardExpression] { selector: '**', predicate: -> false }
    arrive:
      'uid2': [GuardExpression] { selector: '**', predicate: -> false }

Whatever form this takes, in practice I think the transformation will be an important step, since, compared with a flat array of guards, I would expect that having the guard expressions indexed by type (enter arrive etc.) will present an obvious performance advantage during the guard-evaluation tree-walk.