clauderic / dnd-kit

The modern, lightweight, performant, accessible and extensible drag & drop toolkit for React.
http://dndkit.com
MIT License
12.97k stars 643 forks source link

perf regression: all Sortables in 5.0 rerender constantly on even smallest mouse movement #623

Closed dontsave closed 2 years ago

dontsave commented 2 years ago

Hi there. Thanks for this amazing, composable library. We are upgrading from 4.0 to 5.0 and noticed a performance regression. Each Sortable now rerenders constantly on even the smallest mouse movement. This is visible by turning rerender indicators on in react dev tools and visiting an example Sortable story:

https://user-images.githubusercontent.com/3527177/153221284-20794b8f-9384-439a-acf8-25a0b343adf7.mov

In previous versions the Sortables do not rerender unless there is a change on the over state. This would be the expected behavior:

https://user-images.githubusercontent.com/3527177/153223557-ec66c6eb-95ed-4a77-9148-f5a28312cf1f.mov

ccpu commented 2 years ago

The same problem is happening to me, 'useSortable' is causing retenders due to 'useContext'. The easiest way to fix this is to use useContextSelector .

Animation

sean-esper commented 2 years ago

Are you suggesting theres a way to use useContextSelector locally to fix this, or that clauderic should use it in the library

dontsave commented 2 years ago

i would advocate for its use in the library. dndkit is highly dependent on react context, which makes for a clean architecture, but also causes the standard performance issues that come with it (all consumers rerender regardless of what part of the context value changes). i get wanting the keep the library zero dependency, but if there is any wiggle room in that requirement useContextSelector would be the ideal fix. it looks very likely to be adopted natively into react anyway, at which point the library could go back to not depending on anything

ccpu commented 2 years ago

I agree that it should be used in the library to improve performance, but for now I am using 'useContextSelector' locally to resolve the issue.. here is the code:

import {
  ContextSelector,
  createContext,
  useContextSelector,
} from '@fluentui/react-context-selector';

const sortableItemContext = createContext<SortableItemContextStates>(
  {} as SortableItemContextStates,
);
const { Provider, Consumer } = sortableItemContext;

export const useSortableItemContextSelector = <T>(
  selector: ContextSelector<SortableItemContextStates, T>,
) => {
  return useContextSelector(sortableItemContext, selector);
};

export const useSortableItemContext = () => {
  return useContextSelector(sortableItemContext, (c) => c);
};

export const SortableItemProvider = forwardRef<HTMLDivElement, any>(
  (props, ref) => {
    const { itemId, children, ...rest } = props;

    const { attributes, listeners, setNodeRef, transform, transition, isDragging } =
      useSortable({
        id: itemId,
      });

    const sortablePropsMemo = useMemoObject({
      attributes,
      isDragging,
      listeners,
      setNodeRef,
      transform,
      transition,
    });

    return (
      <Provider value={sortablePropsMemo}>
        {React.cloneElement(children as any, { ...rest, ref })}
      </Provider>
    );
  },
);
dontsave commented 2 years ago

@ccpu great bandaid. thanks for that!

Brian-McBride commented 2 years ago

@ccpu Do you have a fully implemented example? I'm pretty new to this repo and looking to bootstrap fast.

I'll second your thought about implementing the right 3rd party lib. I've always felt that React Context is the trading understanding component lifecycle for the questionable ease of Context providers bringing poor performance in general.

While I understand avoiding 3rd party libs, maybe it is possible to offer abstraction to where people can "bring their own" state lib as part of the docs. I personally avoid Context and have found huge success in very small libs like Zustand or Hookstate, superior performance to be sure.


Edit:

I tried the above pattern, using this as the contents of a list:

<SortableContext
  items={items}
  strategy={verticalListSortingStrategy}
>
  {items.map((item, index) => {
    console.log(`${item} :: StageItems`);
    return (
      <SortableItemProvider id={item} key={'provider' + item}>
        <Item key={item} id={item} workbookId={workbookId} />
      </SortableItemProvider>
    );
  })}
</SortableContext>

This is what I see when I am using the example pattern of multiple columns of items.

The above was done using only with items. When I make the columns sortable (nested SortableContext ) then I see basically the same thing. Dragging an item only in its container (a single item in a container alone) follows all the above updates, just adding in a SortableContainerProvider that I created for these as well.

In all this, I was dragging one item to not interact with other items or containers. Obviously, when you do there are more updates that match the pattern.

All I can guess is that, in this case, SortableContext is replacing an object that is being passed to the provider causing all the useSortable hooks to trigger. I'm thinking it is the Provider, not the Hook based on the dates each was modified.

numberinho commented 2 years ago

I'm having the same issue, with a bit more complex items the re-rendering even causes the drag-animation to be laggy-ish, so its not really useable anymore. :( A fix would be highly appreciated! Thank you for developing such a nice library

Arkellys commented 2 years ago

I have the same problem, I updated from from v3 to v5 and everything is very laggy now.
I will try to see if useContextSelector fixes the issue for me.

numberinho commented 2 years ago

It would be awesome if you can post a little more detailed solution if you can fix this. I'm kinda new to this repo and still highly rely on the documentation. Im using the Drag Overlay with ref forwarding pattern approach right now.

tmanson commented 2 years ago

PR #642 fixed in 6.0.1 ?

Arkellys commented 2 years ago

I don't think it's fixed, here is what I have with core 5.0.3 and sortable 6.0.1:

https://user-images.githubusercontent.com/32762018/159113748-9c424829-d803-4466-a220-f1f346fd444f.mp4

And what I have with core 3.1.1 and sortable 4.0.0:

https://user-images.githubusercontent.com/32762018/159113796-b1055906-65fc-4d58-9464-d163436bde5f.mp4

The new version has clearly a performance problem...

clauderic commented 2 years ago

@Arkellys please share actual code, ideally a CodeSandbox replicating the issue. Videos aren't super helpful to help diagnose the root of the issue.

Arkellys commented 2 years ago

@Arkellys please share actual code, ideally a CodeSandbox replicating the issue. Videos aren't super helpful to help diagnose the root of the issue.

Yes, you're right! And it was indeed necessary because after trying to reproduce the issue on CodeSandbox for hours to no avail I endend up removing 99% of my code only to find out that the problem came from my CSS. Somehow, at some point I set up:

* {
   -webkit-app-region: no-drag;
}

I don't remember why I wrote that but it was probably for something related to Electron. Removing it fix the problem and everything is very smooth again! Curiously, I still can't reproduce the problem on CodeSandbox. I also have no idea what changed between the versions of dnd-kit to cause that.

dontsave commented 2 years ago

confirming #642 seems to fix for me!