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

CanFire does not support triggers with parameters #453

Open
lloydjatkinson opened this issue Jun 3, 2021 · 7 comments
Open

CanFire does not support triggers with parameters #453

lloydjatkinson opened this issue Jun 3, 2021 · 7 comments

Comments

@lloydjatkinson
Copy link

lloydjatkinson commented Jun 3, 2021

Currently CanFire has these two overloads:

        public bool CanFire(TTrigger trigger, out ICollection<string> unmetGuards);
        public bool CanFire(TTrigger trigger);

However it does not support triggers with parameters, is there a technical limitation for this? Is there any possibility of adding support for this? It seems strange that I can check if the trigger can be fired for "normal" triggers and get a list of unmet guards, but I can't for triggers with parameters.

As I have a few normal triggers and a few parameterised triggers, the best way I can see for how to achieve checking if it can fire now is to wrap a call to Fire with a try/catch. Not ideal if we can avoid exceptions fully if a new overload was added.

@HenningNT
Copy link
Contributor

I think the CanFire method should be made private, so that's it not available for users of the library.

My reasoning for this is that states are transient. Imagine calling CanFire, which involves checking some information in database. This information might change right after checking it, making the result of CanFire wrong.

I strongly suggest not depending on CanFire during run time. I suppose using it in unit tests is OK, since all information is static in a unit test.

If you use CanFire to make decisions on wether or not to Fire a trigger, I think you are not taking fully advantage of the state machine library.

If you use CanFire to drive a UI, then I suggest splitting your application up, so that you have one state machine dedicated just for the UI. You can hook up the transition events to drive the UI updates.

As to the question if there is a technical reason why CanFire can't accept pamaters, no there is none. It isn't implemented yet because nobody has asked for it yet :-)

@lloydjatkinson
Copy link
Author

I see, thanks 😊 So wrapping Fire with a try/catch to check if something was successful or not is your recommendation? I'm not checking whether to fire or not, I'm simply using CanFire currently to inform the consuming code if it was successful or not.

@josellm
Copy link

josellm commented Nov 10, 2021

I created a pull request about it.

#467

@lloydjatkinson
Copy link
Author

You can hook up the transition events to drive the UI updates.

Did you mean for example, having two FSM's that communicate with each other? Not totally clear

@tibitoth
Copy link

tibitoth commented Mar 9, 2022

@HenningNT Firstly could you make CurrentRepresentation property protected? That would enable more scenarios for authors but not on the public API surface

@SypleMatt
Copy link

+1 for a CanFire() method with arguments.

Sometimes you just want to check if a trigger is possible, such as in a REST API when you're rendering links driven by the state machine. Calling Fire() invokes rules and actions, meaning there are side effects probably including a state transition. Calling CanFire() only invokes rules and so there are no side effects. That's a really handy feature.

Based on the above I'd argue that there is value to having the CanFire() method. Having that method not support parameterised triggers is half an implementation though. The CanFire() method calls this.CurrentRepresentation.CanHandle(trigger), which can take a list of arguments. So it seems like a trivial change (technically).

In terms of @HenningNT's point about states being transient, I don't think that should be an issue. The docs state that a state machine is NOT thread-safe so there's no guarantee being broken here. You already have a potential race condition from the time you initialise the state machine to the time you call Fire(). Supporting a CanFire() method doesn't change that. If my take on this is flawed, or if I'm missing some of the argument, then please elaborate ;)

Alternatively, the suggestion from @tibitoth would be good enough as it would allow me to add the method I consider missing.

@tibitoth
Copy link

@SypleMatt In the mean time I found another workaround with the current version:

public bool CanFire(TTrigger trigger, params object[] arguments)
{
    if (arguments == null || arguments.Length == 0)
        return base.CanFire(trigger);

    return GetPermittedTriggers(arguments).Any(x => Equals(x, trigger));
}

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

No branches or pull requests

5 participants