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

Playground is broken due to recent renderMarkdown + CRLF change #2069

Closed
compulim opened this issue Jun 11, 2019 · 3 comments · Fixed by #2073
Closed

Playground is broken due to recent renderMarkdown + CRLF change #2069

compulim opened this issue Jun 11, 2019 · 3 comments · Fixed by #2073
Assignees
Labels
bug Indicates an unexpected problem or an unintended behavior.

Comments

@compulim
Copy link
Contributor

Screenshots

image

Version

"master"

Describe the bug

Playground throw exception after trying to connect to Mockbot.

To Reproduce

Steps to reproduce the behavior:

  1. Browse to https://webchat-playground.azurewebsites.net/
  2. Click on "Connect to official Mockbot"

Expected behavior

User can start talking to Mockbot.

Additional context

Seems related to recent CRLF changes on renderMarkdown.

[Bug]

@compulim compulim added bug Indicates an unexpected problem or an unintended behavior. Pending labels Jun 11, 2019
@tdurnford
Copy link
Contributor

I'm not exactly sure how the Playground is using the markdown renderer, but the renderMarkdown function takes a second parameter - styleSet - that is deconstructed to access the markdownRespectCRLF value in Web Chat's style options. Perhaps we shouldn't deconstruct the styleSet in the function declaration and check for the options existence in an if statement instead.

@compulim
Copy link
Contributor Author

compulim commented Jun 11, 2019

renderMarkdown should be as simple as (markdown: string) => string, without options.

And people who send in renderMarkdown, e.g. FullReactWebChat, should be responsible to configure the Markdown engine.

The current code in FullReactWebChat is good (i.e. memoizedRenderMarkdown part) because it configure the Markdown engine and send in a function (markdown: string) => string.

We prefer simple interface for renderMarkdown because it would be easily adaptable to other engines without looking into our code.

@tdurnford
Copy link
Contributor

tdurnford commented Jun 11, 2019

Right, so the markdown renderer being passed to ReactWebChat needs to be of type (markdown: string) => string, but the type of the markdwon renderer being exported by botframework-webchat is of type (markdown: string, options: any) => string. It looks like the Playground is importing the renderer from botframework-webchat and then passing it to ReactWebChat causing the type mismatch. This seems like a redundant step since the renderer is configured in FullReactWebChat by default.

Should we reconfigure the render function that botframework-webchat is exporting, remove the renderMarkdown prop passed to ReactWebChat in the Playground, or both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or an unintended behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants