fix: Rewrite view (re-)assignment logic

[ishitatsuyuki]

Description (from primary commit)

This commit rewrites the view (re-)assignment logic in a way that is shorter and therefore easier to read through. The performance is generally on par with the old implementation. It also fixes bugs in the old logic where:

• The item itself getting used as the keyField instead of the index.
• Sometimes a view could get stuck in a corrupted state where its key doesn't match what the index or item indicates.

These bugs were rather hard to reason about in the old logic due to the redundant code path it had (which weakened the invariants). The new logic unifies paths for simplicity, while keeping the time complexity aspect roughly the same.

Testing

I've done some brief tests on this using my own app and it has been working fine, even better because there were some annoying bugs before as described above.

Not all code paths are exercised in my app though, so additional testing would be useful.

Reviewing

This PR is split into a bunch of small refactoring commits that should be trivial to review, and a main commit (fix(RecycleScroller): Rewrite view (re-)assignment logic) after all the refactoring is done. It should be easier to review commit by commit.

[ishitatsuyuki]

@Akryum This resolves most of the critical issues I encountered when using this package in my project. Would appreciate a review!

[ishitatsuyuki]

@Akryum Any chance to get this reviewed?

[ishitatsuyuki]

We're longer setting the type and the used anymore? Confused by this change.

type is only set once on view creation, the point is it should not change during the recycling process or it breaks the assumption of how the recycled views are stored.

Similarly for used, they only change when a recycled view get displayed or vice versa.

[ishitatsuyuki]

@patchthecode Wonder if you had a chance to try this PR? Does it solve your "stale data" issue?

[patchthecode]

I was not able to test it because i think i am using a later vue version?

i ran this command

npm i https://github.com/ishitatsuyuki/vue-virtual-scroller

and here was my results

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: evvrything@0.0.0
npm ERR! Found: vue@3.2.37
npm ERR! node_modules/vue
npm ERR!   vue@"^3.2.37" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer vue@"^2.6.11" from vue-virtual-scroller@1.0.10
npm ERR! node_modules/vue-virtual-scroller
npm ERR!   vue-virtual-scroller@"https://github.com/ishitatsuyuki/vue-virtual-scroller" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR! 
npm ERR! See /Users/zuaaef/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/zuaaef/.npm/_logs/2022-08-27T12_46_28_826Z-debug-0.log

[ishitatsuyuki]

npm i https://github.com/ishitatsuyuki/vue-virtual-scroller

That won't work because source repos doesn't contain the dist artifacts.

I suggest cloning the repo to somewhere, npm build then reference that as a path dependency.

[patchthecode]

for reference, the vue-virtual scroller i used was the one compatible with vue3 as instructed by the repository owner --> https://github.com/Akryum/vue-virtual-scroller/tree/next/packages/vue-virtual-scroller

[ishitatsuyuki]

The branch is logic-rewrite not master.

[patchthecode]

Sorry but my problem is that i am new to NPM, and i do not know how to install from a github repository

npm install --save "https://github.com/ishitatsuyuki/vue-virtual-scroller#logic-rewrite"
^C(##################) ⠧ reify:esbuild-android-64: timing reifyNode:node_modules/esbuild-android-arm64 Compl
npm ERR! git dep preparation failed
npm ERR! signal SIGINT
npm ERR! command /Users/t/.nvm/versions/node/v16.13.0/bin/node /Users/zuaaef/.nvm/versions/node/v16.13.0/lib/node_modules/npm/bin/npm-cli.js install --force --cache=/Users/zuaaef/.npm --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm WARN ERESOLVE overriding peer dependency
npm ERR! npm WARN While resolving: eslint-config-standard@16.0.2
npm ERR! npm WARN Found: eslint-plugin-promise@5.1.0
npm ERR! npm WARN node_modules/eslint-plugin-promise
npm ERR! npm WARN   dev eslint-plugin-promise@"^5.1.0" from the root project
npm ERR! npm WARN 
npm ERR! npm WARN Could not resolve dependency:
npm ERR! npm WARN peer eslint-plugin-promise@"^4.2.1" from eslint-config-standard@16.0.2
npm ERR! npm WARN node_modules/eslint-config-standard
npm ERR! npm WARN   dev eslint-config-standard@"^16.0.2" from the root project
npm ERR! npm WARN 
npm ERR! npm WARN Conflicting peer dependency: eslint-plugin-promise@4.3.1
npm ERR! npm WARN node_modules/eslint-plugin-promise
npm ERR! npm WARN   peer eslint-plugin-promise@"^4.2.1" from eslint-config-standard@16.0.2
npm ERR! npm WARN   node_modules/eslint-config-standard
npm ERR! npm WARN     dev eslint-config-standard@"^16.0.2" from the root project

i only know how to install from npm it self

[ishitatsuyuki]

I said you should clone the repo to some path and then use a path dependency, not a git dependency.

[patchthecode]

I'll have to check this another time then. my lack of understanding of npm and my very flakey setup is making this take some time to install properly. And i'm really low of time right now.

I'll have to revisit this in the future. In the mean time, i have forcefully reloaded by virtual scroller as a work around.

[zsdycs]

@patchthecode To verify this code, you need to build the source code and use it. The following is the general process for reference:

1. Clone this repository using Git
2. Use git checkout to switch the branch to logic-rewrite
3. Build according to the build command described by scripts in package.json
4. Use built local dependencies

[fidalgodev]

I'm having this issue too. See my comment here

[fidalgodev]

Any chance that this can be reviwed and merged? I'm having the issue of the index being out of date after the list we pass to the virtual-scroller is updated a couple of times... @ishitatsuyuki @zsdycs

[ishitatsuyuki]

You need to ping @Akryum not me

[Howard-Lam-UnitedVanning]

Works perfectly. Thank you for the pull

[fidalgodev]

Can this be merged? @Akryum

[alindmtr]

Can this be merged? @Akryum