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

re-rendering ALL draggable items on drag start and such #1071

Open
alissaVrk opened this issue Mar 18, 2023 · 19 comments · May be fixed by #1096
Open

re-rendering ALL draggable items on drag start and such #1071

alissaVrk opened this issue Mar 18, 2023 · 19 comments · May be fixed by #1096

Comments

@alissaVrk
Copy link

alissaVrk commented Mar 18, 2023

it seems that all draggable components will re-render when one of them is dragged, it happens because of the use of context in useDraggable and the way that the context exposes the values, for example, active (which definitely changes when you start dragging).

also need to change from exposing the active element on the context to isDragging or myActiveInfo so that components that aren't active won't get a new value that they don't need (with my solution it will be useIsDraggable or useMyActiveInfo)

 const {
    activators,
    activatorEvent,
    active,
    activeNodeRect,
    ariaDescribedById,
    draggableNodes,
    over,
  } = useContext(InternalContext);

there is a way around it. the idea is to expose functions instead of values on the context value so it won't re-render all components that use it when something that is irrelevant to them changes.

I wrote a blog post on the subject.

it probably happens in many other cases.. like a change in over

If you like the idea I can try and prepare a PR

the problem is also that you expose active to the user of useDraggable and if we do the change I propose non-active components won't have that information - it will change the API a bit

@jonahallibone
Copy link

Same issue here. Anytime I drag start the entire page rerenders. This is a problem on pages with 100+ items, the page freezes for 1s+

@alissaVrk
Copy link
Author

alissaVrk commented Mar 21, 2023

@jonahallibone a workaround for this issue could be creating a Draggable component that will receive the content (the actual component) as children. but then you cannot pass isDraggable and such to it - sandbox
All the Draggable components will still re-render on drag start and such, but the Item won't

Or you can separate the draggable item into 2 components: DraggableItem and Item + memo the Item.
This way you can pass whichever props you like, but it has the disadvantages of memoization - sandbox
All the DraggableItem components will still re-render on drag start and such, but the Item won't

don't get me wrong, I still think that the right thing to do is not to re-render all of them

@jonahallibone
Copy link

@alissaVrk thanks! that helped a bit!

@tomasmenezes
Copy link

@alissaVrk a significant amount of performance-based open issues are both directly and indirectly related to this (e.g., #994, #943, #898, ...). A PR would be highly appreciated!

@alissaVrk
Copy link
Author

@tomasmenezes I will try to create a first draft for next week

@alissaVrk
Copy link
Author

alissaVrk commented Mar 30, 2023

@tomasmenezes PR ready, only for active.
it won't solve most of the issues you mentioned, to solve them we need to change useSortable as well.

useSortable exposes activeIndex and overIndex which will cause a re-render for all items even with my fix.
it is possible to solve but it will require a change in the API.
for example, we can expose over: 'before-me' | 'me' | 'after-me' | 'other' on useSortable and add another hook for a more generic use - useSortableContext which will expose overIndex and such.
There might be more issues there, I didn't dig deep into the sortable yet

@alissaVrk alissaVrk linked a pull request Apr 5, 2023 that will close this issue
@alissaVrk
Copy link
Author

@tomasmenezes PR for drappable and sortable is ready
there are quite a few changes in the API, but I couldn't avoid it..

All items will still re-render if the passed items to the sortable context change, happens mostly on drop, probably fixable as well, but I had to stop at some point :)

tested with the example in #994, seems to solve the issue.
if you know of more tickets with reproductions, let me know, will check the impact

@tomasmenezes
Copy link

@alissaVrk Looks awesome! Will also test it out later this week. I think API changes are welcome to deal with issues as big as this one. I think most people here would encourage you to keep on going 😄

@alissaVrk
Copy link
Author

alissaVrk commented Apr 6, 2023

@tomasmenezes, If this PR will get enough attention to actually get merged, maybe I will :)

@kyeo76
Copy link

kyeo76 commented Apr 6, 2023

@alissaVrk great work! i hope @clauderic will eventually continue to work on this lib. unfortunately there has been no activity for several months

@alissaVrk
Copy link
Author

@tomasmenezes @kyeo76 Thank you for the feedback :)

@stevenkkim
Copy link

@alissaVrk Thanks for working on this! I'm brand new to dnd-kit, but this is the first thing I noticed - that all draggable and droppable components are re-rendering on start, over and end.

@sophia-kravchenko
Copy link

any updates on this?

@sheminanto
Copy link

Any updates on this? I am facing performance issues with sortableContext when there are more than 500 components.

@alissaVrk
Copy link
Author

@sophia-kravchenko @sheminanto doesn't seem like it :(
but you can try the workaround I suggested in the 3rd comment to this issue, it might help a bit

@Julibnk
Copy link

Julibnk commented Jul 11, 2023

I am facing this same issue, thanks @alissaVrk for the workaround and @clauderic for this amazing library!
It would be awesome if the PR finally gets merged in new versions

@Jordan-Eckowitz
Copy link

I've used react-tracked (https://github.com/dai-shi/react-tracked) on projects that use context with great results. It acts as a proxy between the context and components consuming it - only triggering re-renders to components consuming the parts of the context that have changed.

I was curious if anyone has tried it (also, there's a few other libraries that do something similar)?

@MaxtuneLee
Copy link

I've used react-tracked (https://github.com/dai-shi/react-tracked) on projects that use context with great results. It acts as a proxy between the context and components consuming it - only triggering re-renders to components consuming the parts of the context that have changed.

I was curious if anyone has tried it (also, there's a few other libraries that do something similar)?

Great inspiration, I haven’t tried, but I also know that https://github.com/dai-shi/use-context-selector can achieve too. I will try it later today, thanks for the solution.
And I’m also curious about the status of the PR above, since it hasn’t updated for a long time ; (

@ZachMayry
Copy link

@jonahallibone a workaround for this issue could be creating a Draggable component that will receive the content (the actual component) as children. but then you cannot pass isDraggable and such to it

You can if you use cloneElement. I've memoized it to make sure it doesn't constantly clone 100s of elements.

const clonedChild = useMemo(
    () =>
      cloneElement(children, {
        isDragging,
      }),
    [children, id, isDragging]
  )

  return (
    <div
      ref={setNodeRef}
      style={style}
      {...attributes}
      {...listeners}
    >
      {clonedChild}
    </div>
  )

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.