@developit My understanding could be wrong here, but areWeakMaps 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 likehttps://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?

@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.

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

@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 fromh(), the return value corresponding to an evicted mapping will now be escaped text.

At its core, the issue is this:

constlink=vhtml('a',{href:'/'},'hello');console.log(link);// `<a href="/">hello</a>`  <-- now mapped as an allowed stringconstdiv=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.awaitsleep(60);// we pass `link` expecting it to be HTML, but it's no longer in the mapping.constnewHtml=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 fromvhtml() 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.

@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 acleanup function that resets thesanitized 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?

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

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