Rework msg for: "Danger: Expected markup to render 15 nodes, but rendered 10" invariant

[slorber]

Hi,

This invariant message does not give great insight of what is happening. @syranide helped me to find out that this might be related to malformed html content used in dangerouslySetInnerHTML and it actually was the case.

I use an "autolinker" library that transform text links into html links, and it has a bug that produces malformed HTML content (see https://github.com/gregjacobs/Autolinker.js/issues/88) This bug actually broke the whole UI for an user. An unperformant workaround is to parse the string before passing it to React with something like $("<div>" + autolinkerOutput + "</div>")[0].innerHTML It seems to me quite common in frontend apps code to transform or shorten dynamic html content, and any bug in these libraries can seriously mess up with React.

In this case, Danger.dangerouslyRenderMarkup was called with a markupList of size 15. But during this call:

var renderNodes = createNodesFromMarkup(
        markupListByNodeName.join(''),
        emptyFunction // Do nothing special with <script> tags.
      );

Only 10 elements were returned, because of one of the elements being malformed, because the parsing of the browser only produced 10 dom nodes when calling node.innerHTML = markup;

---

I don't really know what to do with this.

Maybe it can be better documented that dangerouslySetInnerHTML can only be used with well-formed HTML, even if the browser can fix the markup.

Maybe the invariant message could be more helpful for the user in this case and telling the user it could be due to dangerouslySetInnerHTML with malformed html.

Maybe this case of malformed HTML could be handled by React itself, but I don't really know how it could be done without performance penalty.

I tried do parse each single markup individually to have the same input/output number with the following code, but It fails later:

      if ( markupList.length !== renderNodes.length ) {
        renderNodes = markupListByNodeName.map(function(singleMarkup) {
          return createNodesFromMarkup(singleMarkup,emptyFunction);
        }).reduce(function(a, b) {
          return a.concat(b);
        });
        console.warn("potential malformed HTML!!!",renderNodes);
      }

[sophiebits]

Thanks for the issue – you're right that we do assume that dangerouslySetInnerHTML markup is valid so we didn't ever expect this error message to fire; it's more of a sanity check. We can at least try to document better and improve the error message, but maybe we can also consider validating the markup in development mode.

[sophiebits]

(We parse them together for performance reasons.)

[syranide]

We can at least try to document better and improve the error message

I believe it was mentioned some time ago, perhaps we should put out a one-time message first time dangerouslySetInnerHTML is used. But then most people probably use it somewhere and will nag to get rid of it... :)

but maybe we can also consider validating the markup in development mode.

I'm not aware of a reasonably fast/easy/small way of validating the markup other than using a parser, but even that would not be 100%.

We could make createNodesFromMarkup more robust and helpful by interspersing all markup with a comment and then validate all comments being in order, but it can't give you the reason/source of the error.

We can't validate the markup out of context either as invalid nesting plays its part too. The only thing that seems realistic to me is wrapping dangerouslySetInnerHTML markup with a comment on each side, then we walk the tree and ensure that all such special comments are on the same level.

But you're always at the mercy of chance here, the root problem is that the user must be made aware that markup must be well-formed, valid in context and safe/sanitized.

[sophiebits]

I believe it was mentioned some time ago, perhaps we should put out a one-time message first time dangerouslySetInnerHTML is used.

That doesn't work in codebases worked on by more than one person. :)

[wattsbn]

For what its worth I also ran into this problem. I don't know how long it would taken me to figure out what was causing it if this issue hadn't shown up in my search results. In my case, I am using an API that I have no control over and for one of the fields it uses raw html.

I think it would be worthwhile to have a link to a wiki page documenting some of the causes (obviously including this one). Some of the other errors and warning do this and I have found it very helpful in figuring out what went wrong and what I need to do to fix the it.

[yiminghe]

You can create your own danger html wrap component (degrade performance though):

https://jsfiddle.net/yiminghe/1hx6b2dn/1/

[zargold]

Is there any work around to get React to ignore this let the HTML be malformed. But this prevents rendering of basically the whole page. When my (safe, admin) user is inputing html they can theoretically make a mistake... How can I catch this error and instead render a <span>This HTML is invalid... Please fix error</span>. Should I just catch it by redefining

console.error = function(){
            for (var i = 0; i < arguments.length; i++){
              if(arguments[i].includes('Invariant Violation: Danger: Expected markup to render')){
                somehow_create_code_that_re_renders_the_dangerouslySetInnerHTML_as_plain()
                somehow_create_code_that_opens_the_editing_modal()
              }
            }
            err.apply(this, arguments)
}

I'm sure someone has already built a much better solution

[sophiebits]

@zargold What version of React are you on? I would expect React 15 to not have this problem since we use document.createElement rather than generating an HTML string.

[sophiebits]

Going to close this since we shouldn't be using this code path any more in most cases.

[zargold]

Hi @spicyj I am using 14.8 I guess I'll try holding my nose and updating. It looks like everything works fine and I can safelyDangerouslySetInnerHTML and no compatibility issues, but we'll see how it fares when it hits regression testing.