💥✨Adjusted State Handling to prevent leaving active Conversational Components

[stammbach]

Proposed changes

Added "active component" checks to toStateIntent, toStatelessIntent & followUpState to prevent leaving the conversational component handler when using those "state" related methods.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  Not sure if it can really be called a breaking change because component are still in development

Checklist

• My code follows the code style of this project
• My change requires a change to the documentation
  I don't think so, because from a user standpoint I assumed to stay inside my component when using states.
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed
  All existing test are running fine for me

[KaanKC]

Hey @stammbach,

first of all thanks for the PR. It points out a bug in the existing components we had created for the upcoming launch.

Generally speaking, especially for components, the state stuff shouldn't be hardcoded. In your case, the component's state is hardcoded as the root state, which is not always the case. Here's an example handler:

{
    // root level of the project's handler.
    ComponentOne: {
        START() {

        },
        ComponentTwo: {
            // nested components are possible
            START() {
                /**
                 * in this case the state would be set to `ComponentTwo.TestState`,
                 * which would throw an error.
                 * The correct path would've been `ComponentOne.ComponentTwo.TestState`
                 */
                this.toStateIntent('TestState', 'TestIntent');
            },
            TestState: {
                TestIntent() {

                }
            }
        }
    }
}

Because of that, the correct way to route around inside a component should be:

this.toStateIntent(`${this.getState()}.TestState`, 'TestIntent');

Thanks for bringing that up otherwise I probably wouldn't have found the bug.

I'd say it's not necessary for toStateIntent() and followUpState() to contain the component stuff. If you work with this.getState() you can already use the routing methods inside a component.

Now in the case of toStatelessIntent() I think it would be useful. But it should set the state to be the current state minus everything until the active component's root state. Example:

// active component: ComponentTwo
// current state: ComponentOne.ComponentTwo.TestState
this.toStatelessIntent('HelloWorldIntent');
// new state: ComponentOne.ComponentTwo

[stammbach]

Hey @KaanKC,
haven't thought about the nested components, sorry! But then there is another problem, because I basically copied the logic from the sendComponentResponse method which means this should also take the deepest root level and not the hardcoded one.

Also, the getState method of setting the state is a great idea, but in my case not really usable because im nesting my states as well. See my example here:

{
    MyComponent: {
        START() {},
        FirstState: {
            START() {},
            Confirmation: {
                Yes() {},
                No() {},
            }
        },
        SecondState: {
            START() {},
            Confirmation: {
                Yes() {},
                No() {},
            }
        }
    },
}

So if I want to go from FirstState.Confirmation to SecondState, the getState() method is not helpful because I need to get one state up. Thats why I was thinking about always taking the root as stating point for state navigation. Maybe a getRootState method should be also available to allow that functionality.

[stammbach]

I think I've come up with a pretty good solution. I've added the getRootState method and implemented it onto delegate, toStateIntent, followUpState & toStatelessIntent. Which means these methods will always use the current root state, e.g. None, ComponentOne, ComponentOne.NestedComponent without any internal states.

nevermind (removed)

I've modified the hello world skill example with some nested components and did some testing. I could publish it if helps with your own testing.

[KaanKC]

But then there is another problem, because I basically copied the logic from the sendComponentResponse method which means this should also take the deepest root level and not the hardcoded one.

sendComponentResponse() uses the stateBeforeDelegate property which is the value of getState() at the time of delegation. That should be the correct path at all times or am I missing something?

So if I want to go from FirstState.Confirmation to SecondState, the getState() method is not helpful because I need to get one state up. Thats why I was thinking about always taking the root as stating point for state navigation. Maybe a getRootState method should be also available to allow that functionality.

Yeah, I see where the issue is. I mean technically that's something you could solve yourself inside the component but maybe providing a helper is the better solution.

I think I've come up with a pretty good solution. I've added the getRootState method and implemented it onto delegate, toStateIntent, followUpState & toStatelessIntent. Which means these methods will always use the current root state, e.g. None, ComponentOne, ComponentOne.NestedComponent without any internal states.

Could you explain your solution in a little bit more detail, please? I feel like we could solve this without making changes to the existing routing functions.

I'd say we should simply add a function returning the full path to the currently active component's root state, which you can then use to call the routing methods. So in your example, it would look like this:

// inside the `FirstState.Confirmation` state and you want to route to `SecondState`:
this.followUpState(`${this.getActiveComponentsRootStatePath()}.SecondState`);

Maybe with a different name for the function since it's pretty long.

[stammbach]

There is no change with sendComponentResponse, because it always returns to the state the delegate was called from.

With my changes applied, toStateIntent, followUpState & toStatelessIntent always start at the (active component) root. Here are some examples:

toStateIntent inside a component (which is the root) just adds the provided state:
FirstComponent -> this.toStateIntent('FirstState', 'START'); -> FirstComponent.FirstState

toStateIntent with a component name inside throws an JovoError because you should not be able to use a component name as state:
FirstComponent -> this.toStateIntent('SecondComponent', 'START'); -> JovoError

toStateIntent from any state starts at the root so I can easily get back to the start of the app/active component:
FirstComponent.FirstState.ConfirmationState ... -> this.toStateIntent('SecondState', 'START'); -> FirstComponent.SecondState

toStateIntent inside a nested component will then use the active component as root
FirstComponent.SecondComponent -> this.toStateIntent('SomeState', 'START'); -> FirstComponent.SecondComponent. SomeState

there are no changes to the component usage which means wherever this.delegate gets called the component response will be send:
FirstComponent.SomeState -> this.delegate('SecondComponent', { onCom..: 'CALLBACK' }); -> FirstComponent.SecondComponent -> this.sendComponentResponse(); -> FirstComponent.SomeState@CALLBACK

I think this is much more seamless and easier to remember. It will always start from the root. So inside my app it always start from the "top" of my app. For a component it always starts at the "top" of the component. Always relying on string literals are not very user friendly, also because I did not expect it to leave my component in the first place.

[stammbach]

I've published my test app, hope this helps with understanding what's going on. @KaanKC

[KaanKC]

Thanks for the explanation as well as the example app. That made things easier to grasp. Now that I understand it, I really like the idea. Thanks for the PR!

I've reviewed your changes and added some comments on the changes I would make. Happy to discuss anything you disagree on 👍

[stammbach]

@KaanKC I did some refinement to the codebase, I kept two old discussions unresolved because of naming-reasons. Waiting for your feedback on that. Otherwise everything should be fine now.

[aswetlow]

How about adding checkStateForInitializedComponentName to the Handler class as a private method?

[stammbach]

Moved it into the Handler class similar to the handle* methods, hope that's what you meant. Otherwise I'm not sure how to do "private" methods, I haven't had many visits to the TS-Camp yet.

[aswetlow]

It was in the right place. Can you put it back into the Jovo class? :)
Sorry, my mistake.

[stammbach]

Moved it back to the Jovo class. Hope everything is alright then. Really looking forward to a release with these changes :)