botframework-sdk icon indicating copy to clipboard operation
botframework-sdk copied to clipboard

Design request for SkillDialogOptions - conversationState attribute

Open scheyal opened this issue 4 years ago • 1 comments

Please complete the investigation and recommended design pattern.

Thanks

From HealthBot:

When creating a skill dialog, SkillDialogOptions should be provided. One of the mandatory attributes of SkillDialogOptions is conversationState (from type ConversationState). When I look into the SkillDialog class, I see that the only use of the conversationState is to save its changes in order keep dialog stack synchronized.

However, In our implementation of the bot we don’t use ConversationState at all, and we save the dialog stack under PrivateConversationState.

My question is why does SkillDialogOptions expect an attribute from type ConversationState and not from type BotState (if it expects a BotState, it can be used even if we save the dialog stack under PrivateConversationState)?

Moreover, why does it expect to get a mandatory state object at all, and doesn’t let me manage the state saves by myself (For instance, we use autosave middleware)?

I’m working with the JS SDK of bot-builder, version 4.10.1.

Subject: RE: Bot Framework V4 - SkillDialogOptions - conversationState attribute

I don't think that casting would be the correct thing to do, you could cast it if PrivateConversationState would have derived from ConversationState, but both types derive from BotState, it may work in JS (I haven't tried this) but in strong typed languages this would throw an invalid cast exception.

Do you support group conversations in your bot? If not, then use ConversationState instead of PrivateConversationState.

If do support group conversations, then maybe you can create your own conversation state class the derives from ConversationState and overrides getStorageKey as the OOTB PrivateConversationState does.

I'll discuss with the team to see if we can do the same at the SDK level (change the base of PrivateConversationState to ConversationState) but, if you don't use group conversations, I still think you should use ConversationState instead.

Gabo

Indeed the casting won’t work in strongly typed languages, but since the only usage of ConversationState in SkillDialog is “saveChanges” (which is implemented in BotState, the common ancestor of PrivateConversationState and ConversationState), it does work and run in JS.

We currently don’t support group conversations, but changing our implementation to use ConversationState seamlessly in prod won’t be simple.

Thank you for raising the issue with you team and for the help.

Guy

scheyal avatar Nov 06 '20 22:11 scheyal

@gabog, was there any resolution on this? Naively it seems possible to amend either the class hierarchy of PrivateConversationState or make the SkillDialogOption property typed as BotState instead of ConversationState as the only method used is defined on the parent class.

joshgummersall avatar Jan 20 '21 23:01 joshgummersall