dnd-kit icon indicating copy to clipboard operation
dnd-kit copied to clipboard

Render only needed sortable items

Open alissaVrk opened this issue 1 year ago • 20 comments

fixes #1071, fixes #994, fixes #898 and probably fixes #943 and others

based on https://github.com/clauderic/dnd-kit/pull/1088 so all changes that were there are here as well

still re-renders all the sortable items if the passed items into context change. probably fixable as well, but I think this is already much better.

it was impossible to do without changes to the API and implementation:

  • getNewIndex is now passed to the context instead of useSortable
  • no more localStrategy, I couldn't find a use case that can't be solved in global strategy (I'm passing id there as well) if you know of one let me know
  • in useDroppable active is defined only if it's over the item
  • useSortable does not expose isSorting 'activeIndexandoverIndex`
  • in useSortable active is defined only for active and over items
  • in useSortable over is defined only for active and over items

all stories seem to work as before with a few changes. added tests to verify which items are re-rendered

** I think that active and over names should be changed to explain if I am over some item or some item is over me and strategy and getNewIndex should be connected, also animateLayoutChanges is very confusing

alissaVrk avatar Apr 05 '23 05:04 alissaVrk

⚠️ No Changeset found

Latest commit: 358d37dfa5757d427ea6bc059df240ae829f784f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Apr 05 '23 05:04 changeset-bot[bot]

@clauderic is there hope for this PR? with some changes probably

alissaVrk avatar Apr 21 '23 13:04 alissaVrk

@alissaVrk Can you publish this changes in npm as a custom modified dnd-kit package?

bryanjtc avatar Jul 21 '23 06:07 bryanjtc

@bryanjtc I'll try. as something like @alissa/dnd-kit?

alissaVrk avatar Jul 25 '23 08:07 alissaVrk

@bryanjtc I'll try. as something like @alissa/dnd-kit?

Sure

bryanjtc avatar Jul 25 '23 16:07 bryanjtc

@hexwit The bug described above was a result of the typo, so adding items to the dependency array solves it.

The only major issue I'm experiencing now is related to the initial rendering of conditionally rendered items - e.g., you have two selectable tabs A and B, each conditionally rendering hundreds of sortable items when clicked. If you click on B, for instance, the initial render of the sortable items associated with that tab usually takes a long time, frequently freezing the app.

@alissaVrk You had mentioned the possibility of bringing further improvements to this already amazing contribution - any idea of what those could be or if you would be willing to implement some of them? Besides unusual rendering, the extreme number of calls to some functions (i.e., collision detection) is also appalling.

tomasmenezes avatar Aug 03 '23 21:08 tomasmenezes

@bryanjtc @tomasmenezes published this PR at @alissavrk/dnd-kit-sortable, @alissavrk/dnd-kit-core etc.

there is a sandbox that uses these versions

I did bump versions but didn't update the readme or anything else..

@tomasmenezes about further improvements, I don't remember exactly, one of them had to do with rendering all items on drop. but what is the point if it doesn't get merged?

I can probably recreate the list of API changes I thought would be nice

I guess we could create a new library based on this one, but I don't really understand how are people expected to find it :) and it will need a real name :)

alissaVrk avatar Aug 04 '23 10:08 alissaVrk

any update on this?

allicanseenow avatar Aug 08 '23 14:08 allicanseenow

@clauderic I'd love to see this merged into a major version of dnd-kit. We're currently using it at my company and are having some rerendering issues related to DndContext. Thanks!

divmgl avatar Aug 11 '23 15:08 divmgl

@alissaVrk Sounds amazing! There is still much to improve. Imo continuing this work is extremely meaningful as it has a huge measurable impact on the community's experience - especially regarding essential performance issues such as this one, that should be quickly addressed. It is frustrating that this hasn't been officially acknowledged or merged, and many others are echoing that sentiment through new posts about the lack of updates, contributions and future direction for the library. Maybe a proper fork or a new library (depending on the arch changes) are indeed the best way to move forward!

tomasmenezes avatar Aug 11 '23 22:08 tomasmenezes

@alissaVrk Hello, can u help me? I use react + vite + ts and I got next error after npm i ur packages [plugin:vite:import-analysis] Failed to resolve entry for package "@alissavrk/dnd-kit-core". The package may have incorrect main/module/exports specified in its package.json.

kirill2400 avatar Sep 28 '23 22:09 kirill2400

@kirill2400 hi, sorry that it took me so long.. maybe you didn't install the core package place take a look at the sandbox using this package

alissaVrk avatar Nov 11 '23 06:11 alissaVrk

@kirill2400 hi, sorry that it took me so long.. maybe you didn't install the core package place take a look at the sandbox using this package

I have a similar problem with your package, apparently something is missing inside the package to make it work correctly with vite as I understand it https://github.com/vitejs/vite/issues/7725

kirill2400 avatar Nov 16 '23 09:11 kirill2400

@alissaVrk @kirill2400 I've identified and "fixed" the issue with Vite.

The packages built and published get built under the name dist/dnd-kit-<package>.esm.js; however, in the published packages, the package.json points the module field to dist/<package>.esm.js.

To "fix" this, I have added the following yarn scripts to my project's package.json file:

"fix-dnd-kit-fork-accessibility": "json -I -f ./node_modules/@alissavrk/dnd-kit-accessibility/package.json -e \"this.module=\\\"dist/dnd-kit-accessibility.esm.js\\\"\"",
"fix-dnd-kit-fork-core": "json -I -f ./node_modules/@alissavrk/dnd-kit-core/package.json -e \"this.module=\\\"dist/dnd-kit-core.esm.js\\\"\"",
"fix-dnd-kit-fork-modifiers": "json -I -f ./node_modules/@alissavrk/dnd-kit-modifiers/package.json -e \"this.module=\\\"dist/dnd-kit-modifiers.esm.js\\\"\"",
"fix-dnd-kit-fork-sortable": "json -I -f ./node_modules/@alissavrk/dnd-kit-sortable/package.json -e \"this.module=\\\"dist/dnd-kit-sortable.esm.js\\\"\"",
"fix-dnd-kit-fork-utilities": "json -I -f ./node_modules/@alissavrk/dnd-kit-utilities/package.json -e \"this.module=\\\"dist/dnd-kit-utilities.esm.js\\\"\"",
"fix-dnd-kit-fork": "yarn fix-dnd-kit-fork-accessibility & yarn fix-dnd-kit-fork-core & yarn fix-dnd-kit-fork-modifiers & yarn fix-dnd-kit-fork-sortable & yarn fix-dnd-kit-fork-utilities"

By running yarn fix-dnd-kit-fork after installing the packages, all the package.json files will be correctly fixed for the accessibility, core, modifiers, sortable and utilities, and vite will work.

It's a hacky solution, but should do the trick until this gets oficially merged in, and you want to use this fork instead of the official dnd-kit release.

Edit: these scripts assume you're using yarn and have the json package installed (at least as a dev-dependency)

PedroPerpetua avatar Dec 16 '23 11:12 PedroPerpetua

@alissaVrk, don't know how this will integrate with the new planned API but would you still be able to generate that list of API changes you had in mind to take these performance improvements further?

tomasmenezes avatar Feb 19 '24 17:02 tomasmenezes

@alissaVrk btw, that performance issue I mentioned earlier in the thread is exemplified in this stress test sandbox. Simply changing the active window takes removeChild for a ride. The instance usually crashes when the console is active, and it doesn't seem like further memoization does much.

tomasmenezes avatar Feb 20 '24 18:02 tomasmenezes

@tomasmenezes hi, sorry it took me so long (I'm not using this lib anywhere, and back to work :) ). I have tested the new experimental branch. and it seems that this issue - that all items render regardless of memoization - doesn't reproduce there.

so once it's production-ready things should be better

alissaVrk avatar Mar 09 '24 07:03 alissaVrk

hi @alissaVrk, @tomasmenezes! @alissaVrk are you using another library?

betofiorani avatar Mar 17 '24 17:03 betofiorani

@alissaVrk Regardless, thank you for all of your insights! They are certainly helping to push the library in the right direction!

tomasmenezes avatar Mar 20 '24 13:03 tomasmenezes