dotnet-state-machine / stateless

A simple library for creating state machines in C# code
Other
5.53k stars 765 forks source link

Expose parametrized triggers #492

Closed Pokis closed 1 year ago

Pokis commented 1 year ago

Hello, this issue is similar to the: dotnet-state-machine/stateless#72

The business case: We are building a very dynamic state machine that is configured dynamically runtime via configuration files submitted by custom users. We want them to be able to map some external command (class) to an action/transition that maps to trigger (with parameters). I noticed they are already stored internally, and storing them externally just duplicates the references to these triggers. We can get them later on dynamically with their registration names. I was considering getting them outside via reflection, but that just seems troublesome and would be easier to expose not just trigger names via availableTrigers, but objects themselves, so parametrized triggers can be accessed. Thoughts?

Would be first submit to an open source, so Im unsure of the process. Code added would be something like:

        /// <summary>
        /// Returns configured Parametrized trigger by the key. 
        /// </summary>
        /// <param name="trigger">Key of the trigger</param>
        /// <param name="triggerWithParameters">The actual trigger object to be returned</param>
        /// <returns>bool whether such trigger is registered</returns>
        public bool TryGetTriggerWithParameters(TTrigger trigger, out TriggerWithParameters triggerWithParameters)
        {
            return _triggerConfiguration.TryGetValue(trigger, out triggerWithParameters);
        }

        /// <summary>
        /// Returns registered parametrized triggers.
        /// </summary>
        /// <returns>registered parametrized triggers</returns>
        public IEnumerable<TriggerWithParameters> GetTriggersWithParameters() => _triggerConfiguration.Values;
mclift commented 1 year ago

Hi @Pokis,

Thanks for taking the time to submit this issue and the PR.

My gut feeling is that this should be a companion method to GetPermittedTriggers() that provides both the available triggers and any parameters required by each one. So far, the library hasn't been designed with the intent of inspecting its configuration at runtime, so I'm apprehensive about introducing helper methods that expose its internals.

Would the approach that @nblumhardt suggested in #72—to create a simple type to hold the parameterized triggers—be applicable to your project?

Pokis commented 1 year ago

Yea, this method lands close to GetPermittedTriggers, that one returns just Keys whereas we need objects as well.

Since the project is VERY dynamic (even the actions, arguments, their data types and everything possible comes to configuration dynamically on runtime), we can store the triggers like in #72, but this seems like a duplication, as it will end up using exactly same logic and storage, so if we have 100 machines in memory, that have 1000 triggers each already in them, they will be stored as exactly same references in the system once more. Thus to minimize the duplication I've thought that exposing the same entity from existing place would be better. Plus there comes a risk with managing them if deviations of any sort are introduced throughout the whole system.

I was thinking to use reflection and get them that way, but I do not like the performance penalties it brings do to extensive use of this functionality in our project (whole system will run based on state machines), thus dropped that idea.

Are there reasons why don't want to expose this beside the fact that it exposes internal configuration? As they can still be accessed with reflection with processing penalties. I was thinking maybe make the class have them as virtual fields in the base and submit that as a PR, so we could create our own child state class that could expose these freely, but that as well felt a bit disruptive to the whole project, and I've noticed people already fighting the issue like in #72 so it wasn't unique to my situation.

Thanks! DJ

mclift commented 1 year ago

Thanks @Pokis,

Are there reasons why don't want to expose this beside the fact that it exposes internal configuration?

It's more a case of trying to provide the right behaviors and to avoid facilitating dependencies on the library's internals.

I've had a chance to look at it in more detail now and I would like to check a couple of things:

Thanks!

mclift commented 1 year ago

This is roughly what I had in mind: https://github.com/dotnet-state-machine/stateless/compare/dev...mclift:stateless:feature/get-detailed-permitted-triggers?expand=1

Pokis commented 1 year ago

Hey @mclift , sorry for late response, was travelling a lot past two weeks.

Seems that would work, as long as we can get a trigger that we can use to in invoking, and the parameters are a big bonus that we can use!

mclift commented 1 year ago

Hi @Pokis, just thought I'd check in to see if the latest release resolved this issue.

Thanks!

Mike

mclift commented 1 year ago

Closing as I believe this has been resolved. Please let us know if this isn't the case.