Static content in <Raw/> disappears after component refresh

[waynebaylor]

Using crank v0.4.0 🎉

I have a component that renders some markdown using <Raw/> and noticed that after refreshing the component the markdown would disappear.

For example, the static content in the second <Raw/> tag will disappear after clicking the button to refresh. However, the dynamic content in the first one will still render correctly.

function* App() {
  let refreshCount = 0;

  const refresh = () => {
    refreshCount += 1;
    this.refresh();
  };

  for (const {} of this) {
    yield (
      <Fragment>
        <div>
          <Raw value={`<h1>Dynamic content ${refreshCount}</h1>`} />
          <Raw value="<h2>Static <em>raw</em> content</h2>" />
        </div>
        <button type="button" onclick={refresh}>
          Refresh
        </button>
      </Fragment>
    );
  }
}

Codesandbox example: https://codesandbox.io/s/raw-tag-content-disappearing-unngm

[brainkim]

@waynebaylor As always, I apologize for the bug, I thank you for your clear reproduction, and I’m humbled by your patience. I know what the cause is, but I’m wondering if you’d like to give a go at fixing it? I’m just asking because I want to empower people to contribute rather than fixing everything myself. I’m happy to help you do this or answer any questions or provide cryptic hints. If you’re not interested, I can have a fix by the end of the tomorrow!

BTW, 0.4 involved a little bit of refactoring so the “Crank from Scratch” article is slightly out of date 😭

[waynebaylor]

@brainkim Yeah sure I'll give it a try.

[waynebaylor]

@brainkim In updateRaw I can see how refreshing the component skips over the conditional and returns ret.value:

function updateRaw<TNode, TScope>(
  renderer: RendererImpl<TNode, TScope, TNode, unknown>,
  ret: Retainer<TNode>,
  scope: TScope | undefined,
  oldProps: Record<string, any> | undefined
): ElementValue<TNode> {
  const props = ret.el.props;
  if (typeof props.value === 'string') {
    if (!oldProps || oldProps.value !== props.value) {
      ret.value = renderer.parse(props.value, scope);
    }
  } else {
    ret.value = props.value;
  }

  return ret.value;
}

What I'm not sure about is where the fix should go. Does updateRaw need to be changed or is it just being passed a bad value (i.e. ret.value is incorrect)?

[brainkim]

@waynebaylor Okay so the issue is that the DOMRenderer is returning a document fragment from the renderer.parse() method. The problem with this (which I stupidly forgot) is that document fragments are one time use. Once you append a document fragment to the DOM, its children are removed so the document fragment itself is emptied.

The second issue is that we do an equality check of the <Raw /> value prop against the new value prop, so that we don’t update the value if the HTML hasn’t changed. What happens is that the document fragment is re-rendered with the spent, empty fragment, and nothing is re-rendered.

An easy fix to this problem would be to remove the equality check from updateRaw(), so that renderer.parse() is called every time the <Raw /> element is re-rendered.

A more complete solution would be to change the return value of renderer.parse() to be a durable value that isn’t a fragment. This would probably involve changing some return types for the internal RendererImpl interface.

[brainkim]

A temporary workaround is to use the new c-static/crank-static prop to make sure the Raw element never rerenders.

<Raw value={html} c-static={true} />

[waynebaylor]

Removing the equality check in updateRaw seems like the most intuitive solution.

Modifying what renderer.parse() returns or having a special case for DocumentFragment seems like it would eventually lead to cloning nodes.

[brainkim]

So one of the potential solutions I was thinking about would be to allow renderer.parse() to return an ElementValue<TNode>. Then you could probably wrap return an array of the DocumentFragment’s children from the parse() method. This would make renderer.parse() a little more flexible in a good way, while also allowing the parse result to always be retained.

[waynebaylor]

I created PR #224 with a change to updateRaw. If you want I can look into changing what renderer.parse() returns, just let me know.

[brainkim]

Fixed and shipped in 0.4.1! Thanks Wade!