Adaptivecards: [JavaScript] Super obnoxious markdown warning

[ryanmccormickspok]

Platform

What platform is your issue or question related to? (Delete other platforms).

• JavaScript

What version are you using? Ex: NuGet 1.0.2, or latest master, etc...
NPM 1.1.3

Issue

Maybe I am missing something and a simple solution exists for this. My issue... the rock solid console.warn (src/card-elements.ts:6316) is extremely obnoxious.

I am working on an application that requires a higher level of security and appreciate that markdown support is excluded by default. What is frustrating is that I am getting punished for not adding it back in. If it is absolutely required to make the library work without a warning, then it should either be added back in (and supported) OR allow for some kind of escape hatch to turn the warning off.

Again perhaps I am missing something and a simple, obvious solution exists but I am not seeing it. Any guidance would be appreciated.

Explain your issue. If you just have a question, please post on StackOverflow instead.

[dclaux]

@ryanmccormickspok I don't think it needs to be a console.warn. Would a console.log be ok?

[matthidinger]

Would something like this work to remove the warning and safely pass the text as-is?

IMO even a console.log would be kind of annoying if someone intentionally does not want to render markdown.

AdaptiveCards.onProcessMarkdown = function(text, result) {
    var p = document.createElement("p");
    p.innerText = text;
    result.outputHtml = p;
    result.didProcess = true;
}

[ryanmccormick]

Commenting from my personal github account. Can it be even more simple? Can the warning be thrown when it is trying to parse something as Markdown? This would make more sense. Then it could be an error vs a warn.

[dclaux]

I'm not following you, sorry. Would a console.log instead of a console.warn work for you? Or would that still be too much?

[matthidinger]

The reason I originally made it a warning was to be very obvious that "a step is incomplete", and until complete, could have unintended side effects if a card unexpectedly has markdown in it.

For example, a developer rolls out adaptive card support and all looks good, until a user sometime later sees unexpected asterisks in the rendered card, as seen here:

Following the "principle of least surprise" I thought it would be best to make this warning up front, so a developer would have to opt-in to the potential odd behavior. If we waited until parsing markdown, then a developer could finish rolling out a deployment and not find out about the potential bug until much later.

And I believe the code to opt-in to the behavior you want is below. If so, we can include it in the docs that the warning links to, so people don't have to go hunting around like you did :)

Can you let us know if this meets your needs?

AdaptiveCards.onProcessMarkdown = function(text, result) {
    var p = document.createElement("p");
    p.innerText = text;
    result.outputHtml = p;
    result.didProcess = true;
}

[ryanmccormick]

I don't like the document.createElement aspect of the solution as It heavily relies on the browser API. Can I just return the text? Still feels like a hacky workaround. Why not just break the app and throw a hard error when trying to parse markdown without a library? A developer would notice the dependency on a markdown parser right away. If they have unit testing, it should also fail in a CI pipeline and hopefully prevent a breaking release.

[dclaux]

Hm @matthidinger indeed my code didn't include that warning, I was wondering how it got there. I really don't think this warning is necessary. If anything, a simple log entry is enough.

[ryanmccormick]

@dclaux my suggestion is to remove the log completely. Unless absolutely necessary to parse markdown, there should be nothing logged out.

[dclaux]

@ryanmccormickspok I agree with that, I do not believe it is necessary.

[shalinijoshi19]

@dclaux is this something we can tackle for 1.2? (also FYI i assigned to you)

[dclaux]

If we all agree it's fine to simply remove the message, it can be done in 1 minute.

[matthidinger]

@RebeccaAnne raised a point yesterday that this is really an artifact of us not implementing Markdown support in the library, which is tracked by #910

Considering we can use the same code from the ReactNative renderer it seems like the best course of action would be to simply implement markdown natively and neither a warning or log would be necessary.

@ryanmccormickspok since Markdown is a standard part of adaptive cards we don't currently offer a way to easily disable it any of our SDKs. Do you still need the ability to disable it, even if it did not require a third-party library?

[ryanmccormickspok]

@matthidinger as far as I understand, markdown was included and supported in Adaptive cards in the past. The task of providing markdown support has been handed over to the developer and if a developer does not provide a library, a big ugly console warn shows up everywhere - browser console, CI logs and unit tests. When trying to follow a TDD pattern of development, I get to see tests passing with several reminders that I haven't yet supplied a markdown library. I could disable my warn logs, but what if my app used warn? I would miss it completely.

My ask is: offer a way to opt out of the logging. As a developer, I have seen the log statement and understand what is happening so let me pass null or some kind of noop function to disable the log statement for this.

As a side note:
Thank you to everybody for taking time to review this issue and for your contributions to this library.

[matthidinger]

@ryanmccormickspok our goal is to provide markdown support natively in every library without requiring the developer to do anything. The warning was added as a stop-gap until we are able to do that. Once markdown is natively supported per #910, the warning will go away and everything will behave as expected.

In the meantime, until #910 is closed, my personal recommendation would be to implement the markdown processing yourself using the DOM APIs to sanitize it and make sure you won't get injected HTML. That will make the warning go away entirely, but indeed it relies on the DOM APIs that you may need to stub in a test environment.

AdaptiveCards.onProcessMarkdown = function(text, result) {
    var p = document.createElement("p");
    p.innerText = text;
    result.outputHtml = p;
    result.didProcess = true;
}

If that won't work, I won't object to @dclaux changing it to a console.log, but I do worry that users of the library will not immediately realize that a crucial feature of Adaptive Cards is not working as intended, and they may not catch it until sometime later and have to do a patch release. Removing the message entirely, as per the last suggestion, I really have strong feelings against, as now a developer has no idea whatsoever that a hard-to-discover feature is not working correctly.

[dclaux]

our goal is to provide markdown support natively in every library without requiring the developer to do anything.

@matthidinger that has actually never been the goal for the JS library. The main reason has always been that apps may already be loading their own Markdown processing library (Markdown-it or another) and do not want to pay the price of a larger Adaptive library with duplicate Markdown handling logic. The overall size of the JS library is very important to many, especially on mobile devices (yes, some mobile apps use HTML to render Adaptive Cards and not the native renderers).

That being said, I tend to agree with Matt on the resolution: implement your own markdown handler as suggested above.

[shalinijoshi19]

@ryanmccormickspok could you take a look at @matthidinger + @dclaux 's recommendation above please and let us know if that ends up working for you ?

[ryanmccormickspok]

The tl;dr is that it didn't work. It ended up rendering out a bunch of gibberish "[object HTMLParagraphElement]". Maybe from built-in html sanitization?

I ended up building:

function renderWithoutMarkdown(text, result) {
  result.outputHtml = text;
  result.didProcess = true;
}

This was able to snuff out the logging but I am super paranoid about the lack of string sanitization so I am not comfortable using it in prod.

For time purposes, I ended up adding a markdown library.

It would be great if this issue could get solved because a library like this should work without any configuration. Added configuration should enhance/replace basic functionality. Maybe include thin markdown support and then provide something more feature-rich if added through configuration.

I understand the idea around shipping a smaller library but if a developer needs to add third-party markdown support to get it to work in it's most basic state, something isn't quite right.

I still feel like this is an issue but maybe something of a larger architecture scope.

[dclaux]

At this point we think we'll eventually have a built-in Markdown processor, but I can't say when that'll happen.

[msftbot[bot]]

This issue has been automatically marked as stale because it has not had any activity for 5 days.