search

holtwick / zeed-dom

🌱 Lightweight offline DOM
MIT License
33 stars 5 forks source link

zeed dom is collecting all JSX elements it ever used in `USED_JSX` #2

Closed floriankrueger closed 2 years ago

floriankrueger commented 2 years ago

While digging into this issue with tiptap, my colleague @jcarvalho and I found the global variable USED_JSX to be the issue. Even serializing a simple document to HTML in a loop would quickly become slower and slower until it takes over a second to finish.

Memory was not an issue in our tests since the contents of USED_JSX where just the same elements over and over again so node itself could easily optimize that.

tiptap issue #2148 lists an example on how to make this happen. Would be great if you could give an estimate on how easily this might be fixable. I tried switching it from a global var to a parameter yesterday but there are so many points where I needed to pass the parameter to, and some of them were getters or setters, so that wasn't achievable.

holtwick commented 2 years ago

Hi @floriankrueger I'll take a look. Thanks for investigating.

holtwick commented 2 years ago

I made a quick change, to improve the speed of the USED_JSX part. I'll further investigate, if it is required in that form at all or if it can be replaced by something smarter and more consistent. I'll keep this bug open until the evaluation finished. Thanks.

floriankrueger commented 2 years ago

Thank you for the intermediate fix! It'll improve the linear search a lot which should solve the reason for the performance issues that we were experiencing.

One concern remains even after the fix: Renders of previous documents are influencing upcoming documents. So the true returned by the check if (USED_JSX[c] … is incorrect at best and a minor security issue at worst (given that this is deciding on whether or now a certain string is escaped or rendered as html). Would you agree on that or am I misinterpreting something here?

holtwick commented 2 years ago

I agree, this is bad design. I'll fix it but will need some time to do it well.

holtwick commented 2 years ago

I removed the hack around USED_JSX altogether now. @floriankrueger , this should fix the concerns around global usage and also memory consumption. The code was old and made for a special purpose, now it should be up-to-date again. ;)

Change: https://github.com/holtwick/zeed-dom/commit/b88ae22216117d6b8ce39d5723593bfcb97bed64

floriankrueger commented 2 years ago

This is amazing, thank you so much for the time!

holtwick commented 2 years ago

No problem, thanks for pointing on the error and testing to make zeed-dom finally become a "Happy DOM" ;)