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

Strengthen isKeyed test for swap rows #694

Closed
3 of 12 tasks
krausest opened this issue Feb 14, 2020 · 18 comments · Fixed by #759
Closed
3 of 12 tasks

Strengthen isKeyed test for swap rows #694

krausest opened this issue Feb 14, 2020 · 18 comments · Fixed by #759

Comments

@krausest
Copy link
Owner

krausest commented Feb 14, 2020

As discovered in #693 currently the isKeyed test for swap rows is too weak to detect implementations that remove the two rows and simply insert two new rows.
I really think it makes much more sense to require a keyed implementation to actually move the rows, i.e. add exactly the rows that were removed (otherwise any dom state would be lost when swapping rows).

If isKeyed checks whether the rows are actually moved for swap rows the following implementations fail:

  • aurelia-v1.3.0-keyed
  • binding.scala-v10.0.1-keyed
  • crui-v0.1.0-alpha.13-keyed
  • datum-v0.12.2-keyed
  • dojo-v6.0.4-keyed
  • fidan-v0.0.23-keyed
  • maquette-v3.3.0-keyed
  • mikado-v0.7.57-keyed
  • ractive-v1.3.6-keyed
  • react-hookstate-v16.8.6 + 1.5.2-keyed
  • reflex-dom-v0.4-keyed
  • san-v3.7.7-keyed

Can you please take a look at your implementations? Is it possible to fix it for your frameworks?
@Alexander-Taran @Atry @iazel @MartinRixham @agubler @ismail-codar @johan-gorter @ts-thomas @alexfmpe @errorrik

@Atry
Copy link
Contributor

Atry commented Feb 14, 2020 via email

@leeoniya
Copy link
Contributor

the name of the metric has always been "swap", unambiguously. this simply improves the assertion of the metric.

@krausest
Copy link
Owner Author

@Atry I don't think we're changing the definition of keyed, but indeed we're changing the test for keyed that wasn't sufficient (and I'm not sure it's now watertight).

evs-chris added a commit to evs-chris/js-framework-benchmark that referenced this issue Feb 14, 2020
@johan-gorter
Copy link
Contributor

Maquette does not support swapping nodes, you can exclude maquette from this test for this reason.

utkarshkukreti pushed a commit to utkarshkukreti/js-framework-benchmark that referenced this issue Feb 27, 2020
utkarshkukreti pushed a commit to utkarshkukreti/js-framework-benchmark that referenced this issue Feb 27, 2020
@krausest
Copy link
Owner Author

I decided to update the results table such that it show any open issues for the implementations. The information is kept in package.json (js-framework-benchmark.issues).

@ts-thomas
Copy link
Contributor

ts-thomas commented May 5, 2020

Hello @krausest, the mikado keyed implementation was tagged with the issue "Keyed implementations must move the DOM nodes for swap rows", but that's not right. The implementation must assign the new values to the data array (like all others).

.route("swaprows", () => {
    const tmp = store[998];
    store[998] = store[1];
    store[1] = tmp;
})

Edit: Sorry but I misunderstood the issue, I thought that the description explains the issue not the desired behavior.

@krausest
Copy link
Owner Author

krausest commented May 5, 2020

@ts-thomas Please try the following: Click on "Create 1,000 rows", open chrome's dev-tools and set the background color of the row with id 2 to red and click on "Swap rows". If you look now at the second to last row (which has now id 2) is it still red? If not it's not keyed.

@krausest
Copy link
Owner Author

@agubler I just noticed that the swap operation is non-keyed again for dojo 8 alpha 1.Can you take a look at it?

@agubler
Copy link

agubler commented Jan 22, 2021

@krausest have you updated the test?

@krausest
Copy link
Owner Author

@agubler No. I wonder too how that happened. When updating a library I'm running the isKeyed test automatically. So either I ignored the error or it wasn't there at that time.
When checking chome 88 results I ran isKeyed for all frameworks and I got

Frameworks that will be checked dojo-v8.0.0-alpha.1-keyed
Keyed test for swap failed. Swap must add the TRs that it removed, but there were 2 new nodes
dojo-v8.0.0-alpha.1-keyed is keyed for 'run benchmark' and keyed for 'remove row benchmark' and non-keyed for 'swap rows benchmark' . It'll appear as non-keyed in the results
ERROR: Framework dojo-v8.0.0-alpha.1-keyed is not correctly categorized

@agubler
Copy link

agubler commented Jan 22, 2021

@krausest very strange. I’ll try and take a look over the weekend

@krausest
Copy link
Owner Author

@agubler Thanks. (I checked the issue manually in the dev tools. If you assign a background-color style in the dev tool to row #2 and then swap the background color it is lost. So the isKeyed check is correct.)

@agubler
Copy link

agubler commented Jan 22, 2021

@krausest thank you for double checking

@agubler
Copy link

agubler commented Jan 22, 2021

@krausest Hmmm this is strange I have just pulled the latest, re-installed deps, done re-builds and ran npm run check keyed/dojo and it passed! Did I do something wrong? This is on chrome 87 though, I'll try chrome 88

http://localhost:8080/frameworks/keyed/dojo/package-lock.json
Frameworks that will be checked dojo-v8.0.0-alpha.1-keyed
dojo-v8.0.0-alpha.1-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

Edit: Passes in 88 too

@krausest
Copy link
Owner Author

Something is indeed very strange. The version in my docker container behaves non-keyed, when I build it directly on my linux machine it behaves keyed. The package-lock.json is the same for both (but I tried to clean up package-lock.json handling in this release so I'm double checking if I did something wrong there). I'll investigate and report back.

@krausest
Copy link
Owner Author

The build script delete many output directories, but not output. Dojo uses a directory output. I noticed that on my docker container the output directory contains multiple bootstrap..js and main..js files:

total 616
-rw-r--r-- 1 root root     70 Jan 22 21:22 bootstrap.0b93815727c9831545bc.bundle.css
-rw-r--r-- 1 root root    102 Jan 22 21:22 bootstrap.0b93815727c9831545bc.bundle.css.map
-rw-r--r-- 1 root root   8741 Jan 22 21:22 bootstrap.b81f915a0b0556427479.bundle.js
-rw-r--r-- 1 root root  45544 Jan 22 21:22 bootstrap.b81f915a0b0556427479.bundle.js.map
-rw-r--r-- 1 root root   8741 Jan 22 21:22 bootstrap.bfe59d369934154bc858.bundle.js
-rw-r--r-- 1 root root  45544 Jan 22 21:22 bootstrap.bfe59d369934154bc858.bundle.js.map
-rw-r--r-- 1 root root     70 Jan 22 21:22 bootstrap.f484fefd59c714a3fe28.bundle.css
-rw-r--r-- 1 root root    102 Jan 22 21:22 bootstrap.f484fefd59c714a3fe28.bundle.css.map
-rw-r--r-- 1 root root    532 Jan 22 21:22 index.html
-rw-r--r-- 1 root root  31406 Jan 22 21:22 main.9014b7391656d8c72de7.bundle.js
-rw-r--r-- 1 root root 200795 Jan 22 21:22 main.9014b7391656d8c72de7.bundle.js.map
-rw-r--r-- 1 root root  32477 Jan 22 21:22 main.b0bad89716edfc7e672a.bundle.js
-rw-r--r-- 1 root root 207891 Jan 22 21:22 main.b0bad89716edfc7e672a.bundle.js.map
-rw-r--r-- 1 root root   1792 Jan 22 21:22 manifest.json
drwxr-xr-x 2 root root   4096 Jan 22 21:22 runtime

The index.html references bootstrap.b81f915a0b0556427479.bundle.js

When I delete the output folder and let it rebuild in the docker container has the following files:

total 324
drwxr-xr-x 3 root root   4096 Jan 22 20:27 ./
drwxr-xr-x 4 root root   4096 Jan 22 20:27 ../
-rw-r--r-- 1 root root   8741 Jan 22 20:27 bootstrap.bfe59d369934154bc858.bundle.js
-rw-r--r-- 1 root root  45544 Jan 22 20:27 bootstrap.bfe59d369934154bc858.bundle.js.map
-rw-r--r-- 1 root root     70 Jan 22 20:27 bootstrap.f484fefd59c714a3fe28.bundle.css
-rw-r--r-- 1 root root    102 Jan 22 20:27 bootstrap.f484fefd59c714a3fe28.bundle.css.map
-rw-r--r-- 1 root root    532 Jan 22 20:27 index.html
-rw-r--r-- 1 root root  32477 Jan 22 20:27 main.b0bad89716edfc7e672a.bundle.js
-rw-r--r-- 1 root root 207891 Jan 22 20:27 main.b0bad89716edfc7e672a.bundle.js.map
-rw-r--r-- 1 root root   1792 Jan 22 20:27 manifest.json
drwxr-xr-x 2 root root   4096 Jan 22 20:27 runtime/

the index.html references bootstrap.bfe59d369934154bc858.bundle.js. This version behaves correctly even in the docker build.
@agubler Sorry for the confusion - it's not dojo's fault. But I really wonder how and why I got things mixed up that badly.

I'll remove the issue for dojo again and update the official results.

@agubler
Copy link

agubler commented Jan 22, 2021

@krausest oh how funny! Thanks for working that out 😃

@krausest
Copy link
Owner Author

It's been more than two years since this issue was found. I'll move all implementations with 694 to non-keyed for the chrome 102 run.

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

Successfully merging a pull request may close this issue.

6 participants