search

krausest / js-framework-benchmark

A comparison of the performance of a few popular javascript frameworks
https://krausest.github.io/js-framework-benchmark/
Apache License 2.0
6.52k stars 811 forks source link

Reflex DOM benchmark #1626

Closed zouloux closed 1 month ago

zouloux commented 2 months ago

Hi, Back on track ! I just saw that another framework is named "reflex-dom" ... Tell me if I need to rename it ? Or mark it associated to "zouloux". Associated to this ticket : https://github.com/krausest/js-framework-benchmark/issues/1409

alexfmpe commented 2 months ago

Huuuh tricky. The other "reflex-dom" is an haskell framework, so maybe we should rename that one since the default is javascript/typescript? Say, show it as reflex-dom (haskell) in the UI? I don't think it matters much what the folder it's in is called, since not many people will go looking for it in the file tree. Its presence here is also long over due for dust off and simplification which I hope to do soon-ish

krausest commented 2 months ago

Thanks. Seems like google results show the haskell framework first: https://www.google.com/search?q=reflex-dom

We could rename the existing non-keyed implementation to reflex-dom-frp and I'd prefer if you renamed yours reflex-dom-zouloux or reflex-dom-js. Does that sound like a fair solution to you?

But there's an issue in your implementation: It isn't really keyed. The swap operation must move the dom nodes. Can you take a look at that?

zouloux commented 2 months ago

@alexfmpe, @krausest in fact my lib is named "reflex" but on NPM I'm at "reflex-dom" because of availability. I can rename it "reflex-dom-js" or "reflex-js", seems fair ! I'll look to change the name completely later for something not used. @krausest The swap operation uses insertBefore to move node index from parent. On swap, we have 2 insertBefore instructions. Should I change the implementation ?

Screenshot 2024-03-17 at 11 35 36 Screenshot 2024-03-17 at 11 36 59
krausest commented 2 months ago

It's just important that you actually move the node when swapping and not remove and add new ones. (If I don't trust the npm isKeyed keyed/reflex in webdriver-ts check, I apply a background-color to the row that's going to be swapped. If swapping preserves the bg color in the new position it should be keyed).

zouloux commented 2 months ago

I updated my code and ran the isKeyed script ( didn't knew about that, thanks ) : reflex-dom-v0.21.2-keyed is keyed for 'run benchmark' and keyed for 'remove row benchmark' and keyed for 'swap rows benchmark' . It'll appear as keyed in the results But now when I run the benchmark, I have NaN in the swap category. Any idea ? ( the swap seems to work, I also checked with the manual background-color override from the dev tools ). I tested to remove raw results and re-run. Thanks

image
zouloux commented 2 months ago

( the code isn't updated on the pull request )

krausest commented 2 months ago

If you commit the code I‘ll take a look at the NaN issue. I‘ve no idea so far. Sometimes a benchmark fails when the chrome trace doesn‘t contain all data, but I‘m not sure if that happened (there should be some error message on the console then).

zouloux commented 2 months ago

I was swapping the wrong index. Pushing the update soon

zouloux commented 2 months ago

It should be good. Renamed to reflex-js I can move it again if needed. It's categorised as keyed with isKeyed, and swap is not NaN anymore. Thanks !