Strengthen isKeyed test for swap rows

[krausest]

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]

I think it’s better to create a third category other than keyed and unkeyed instead of changing the definition Stefan Krause <notifications@github.com>于2020年2月14日 周五下午12:20写道：

…

As discovered in #693 <#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 <https://github.com/Alexander-Taran> @Atry <https://github.com/Atry> @iazel <https://github.com/Iazel> @MartinRixham <https://github.com/MartinRixham> @agubler <https://github.com/agubler> @ismail-codar <https://github.com/ismail-codar> @johan-gorter <https://github.com/johan-gorter> @ts-thomas <https://github.com/ts-thomas> @evs-chris <https://github.com/evs-chris> @alexfmpe <https://github.com/alexfmpe> @errorrik <https://github.com/errorrik> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#694?email_source=notifications&email_token=AAES3OSSZNJEDQGHZXGP6A3RC34JZA5CNFSM4KVPEJSKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4INVNPLQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAES3OQPU6I57ECUPVR2JRLRC34JZANCNFSM4KVPEJSA> .

[leeoniya]

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

[krausest]

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

[johan-gorter]

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

[krausest]

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]

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]

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

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

@krausest have you updated the test?

[krausest]

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

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

[krausest]

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

@krausest thank you for double checking

[agubler]

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

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]

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]

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

[krausest]

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.