fix(v2): render issue

[Varixo]

This PR fixes render issue

[Varixo]

@mhevery can we replace this code with just escapeContent?

[mhevery]

Yes, but the previous code was way more efficient. RegExp are a bit expensive. So it would be better for streaming if we could fix up the old code to take this into account (OR to rewrite the escapeContent to not use RegExp) Normally I would not care but invoking the RegExp for each text node will add up. Do you need help rewriting this?

[Varixo]

I wrote this code:

    const escapeCharacterMap = new Map([
      ['<', '&lt;'],
      ['>', '&gt;'],
      ['&', '&amp;'],
      ['"', '&quot;'],
      ["'", '&#39;'],
    ]);
    let data = '';
    for (const character of text) {
      const escapeCharacter = escapeCharacterMap.get(character);
      if (escapeCharacter) {
        this.write(data);
        this.write(escapeCharacter);
        data = '';
      } else {
        data += character;
      }
    }
    if (data.length) {
      this.write(data);
    }

Do you have another idea how to do this?

[mhevery]

Map look ups are also expensive. You want to do a scan.

export function escapeHTML(html: string): string {
  let escapedHTML = "";
  const length = html.length;
  let idx = 0;
  let lastIdx = idx;
  for (; idx < length; idx++) {
    // We get the charCode NOT string. String would allocate memory.
    const ch = html.charCodeAt(idx);
    // Every time we cancat a string we allocate memory. We want to minimize that.
    if (ch === 60 /* < */) {
      escapedHTML += html.substring(lastIdx, idx) + '&lt;'
    } else if (ch === 62 /* > */) {
      escapedHTML += html.substring(lastIdx, idx) + '&gt;'
    } else if (ch === 38 /* & */) {
      escapedHTML += html.substring(lastIdx, idx) + '&amp;'
    } else if (ch === 34 /* " */) {
      escapedHTML += html.substring(lastIdx, idx) + '&quot;'
    } else if (ch === 39 /* ' */) {
      escapedHTML += html.substring(lastIdx, idx) + '&#39;'
    } else {
      continue;
    }
    lastIdx = idx + 1;
  }
  if (lastIdx === 0) {
    // This is most common case, just return previous string no memory allocation.
    return html;
  } else {
    // Add the tail of replacement.
    return escapedHTML + html.substring(lastIdx);
  }
}

it('escapeHTML', () => {
  expect(escapeHTML("text")).toEqual("text");
  expect(escapeHTML("<div a='b' c=\"d\">text")).toEqual("&lt;div a=&#39;b&#39; c=&quot;d&quot;&gt;text");
});

[mhevery]

I know this is MORE code, but it will run way faster at runtime.

[mhevery]

Yes, but the previous code was way more efficient. RegExp are a bit expensive. So it would be better for streaming if we could fix up the old code to take this into account (OR to rewrite the escapeContent to not use RegExp) Normally I would not care but invoking the RegExp for each text node will add up. Do you need help rewriting this?