Switch to a WeakMap to avoid memory leak

[developit]

This should address the issue raised in #20.

[AndrewLeedham]

@developit My understanding could be wrong here, but are WeakMaps not intended for storing data about an object that may be garbage collected? Here it seems you are defining a cache of sorts for escaped html which will be a String primitiv.

Perhaps using something like https://www.npmjs.com/package/lru-cache would be a better alternative. Or in the interest of not adding external dependencies either removing the caching or just limiting the stored cache to the last 50 items?

[developit]

@AndrewLeedham yes, Map would be totally fine here. There is very little reason to evict entries anyway. Really the original need is for a cleanup or eviction that happens immediately at the end of a rendering pass.

[AndrewLeedham]

@developit would a conventional Map not have the same issues as an object? I.e it would be defined when vhtml is imported at runtime then keep adding strings to it while the server is alive?

[developit]

@AndrewLeedham apologies, you're right. The problem is that raw-to-escaped mappings persist indefinitely. It's an interesting issue though, because there are performance advantages attributable to keeping these mappings around across renders when strings are repeated, however the memory consumption tradeoff is currently unbounded.

The problem with something like an LRU here is that evicting a string from the map actually changes the output of vhtml() - instead of producing HTML from h(), the return value corresponding to an evicted mapping will now be escaped text.

At its core, the issue is this:

const link = vhtml('a', { href: '/' }, 'hello');
console.log(link);
// `<a href="/">hello</a>`  <-- now mapped as an allowed string

const div = vhtml('div', {}, link);
console.log(div);
// `<div><a href="/">hello</a></div>`  <-- `link` is in the map, doesn't get escaped

// Imagine enough time passes that `link` is evicted from the string mappings.
await sleep(60);

// we pass `link` expecting it to be HTML, but it's no longer in the mapping.
const newHtml = vhtml('p', {}, link);

console.log(newHtml);
// `<div>&lt;a href=&quot;/&quot;&gt;hello&lt;/a&gt;</div>`  <-- the value of `link` gets escaped

I'd previously experimented with returning an opaque value from vhtml() with a toString() method. With that in place, the "children" passed to vhtml are identifiable as having been generated by another vhtml call, avoiding the need for a mapping altogether. However, the performance implications of returning a non-string type from such a hot method were severe.

[AndrewLeedham]

@developit that makes a lot of sense, thanks for the explanation. I would have thought at least from an SSR perspective the performance hit would be preferential over a memory leak. Given React and Preact do this already I can't see the performance being that much of an issue.

Perhaps this could tie in with #6. Whereby 2 packages are provided, the current implementation is kept, but we expose a cleanup function that resets the sanitized cache, then if a user wants improved performance they can use that and call cleanup after a render. And the second involves a performance hit, but no manual cleanup?

[AndrewLeedham]

@developit Perhaps to keep things simple for now, a cleanup function could be exposed so SSR users can clear the sanitized store after each complete render?

[johannesodland]

Tests now fail on this branch, as string primitives can't be used as keys in WeakMaps:

TypeError: Invalid value used as weak map key