Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isKeyed test appears to be wrong for qwik #1496

Open
krausest opened this issue Nov 12, 2023 · 4 comments
Open

isKeyed test appears to be wrong for qwik #1496

krausest opened this issue Nov 12, 2023 · 4 comments

Comments

@krausest
Copy link
Owner

The isKeyed test claims that qwik is non-keyed. Adding a background-color on the tr shows that the dom state is properly in sync with the data for create, remove and swap (or the external state is nicely moved?)

npm run isKeyed -- --headless true keyed/qwik
Keyed test for create rows failed. Expected that 1000 TRs should be removed and added, but there were 0 added TRs and 0 were removed
Keyed test for remove failed. Expected that the dom node for the 2nd row would be removed, but it wasn't
qwik-v1.2.17-keyed is non-keyed for 'run benchmark' and non-keyed for 'remove row benchmark' and keyed for 'swap rows benchmark' . It'll appear as non-keyed in the results
ERROR: Framework qwik-v1.2.17-keyed is not correctly categorized

Until resolved we'll add qwik as keyed. Any ideas why the test fails?

@krausest krausest mentioned this issue Nov 12, 2023
@krausest
Copy link
Owner Author

krausest commented Nov 12, 2023

Qwik replaces the whole table when replacing all rows. No other implementation came up with that solution so far.
And it appears to keep removed elements in a template tag. isConnected is false then, but the tr is still in a template element.

@wmertens
Copy link
Contributor

No other implementation came up with that solution so far

so is that good or bad 😆

@krausest
Copy link
Owner Author

I can't say that - currently it's just strange.
Here's what happens when you click 10x create 10k , the run GC and then create 10k again followed by a GC:
Screenshot 2023-11-25 at 10 16 48 AM
That's a lot of memory, but no severe memory leak (so at least it's not bad)
Same for vanillajs:
Screenshot 2023-11-25 at 10 18 07 AM

@syduki
Copy link
Contributor

syduki commented Nov 25, 2023

so is that good or bad 😆

it depends, for benchmark purposes it is a clever cheat, for real-world applications it is an awful feature/behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants