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

Avoid re-rendering children on click event #1494

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dinkinflickaa
Copy link

@dinkinflickaa dinkinflickaa commented Sep 29, 2024

I noticed that clicking on a DnD child node can be costly because it triggers all child nodes to rerender twice. This issue has been reported before. See #1379

This happens when DndContext updates the activeSensor and activatorEvent states on a mouse down event. These state changes cause rerenders in all consumers of DndContext. Consumers of useSortable re-render too because SortableContext depends on DndContext.

If the user is just clicking or if activation constraints aren’t met, DndContext resets these states to null, causing another render cycle.

Here’s what it looks like in React Profiler for a click event, where you'll see back-to-back commit cycles causing unnecessary rerenders for every child:
image

I don't see why we need to update state unless the user is actually dragging. This PR proposes delaying the activeSensor and activatorEvent state updates until the drag event starts and constraints are met. This reduces unnecessary re-renders, saving two render cycles for every child.

Here’s what the React Profiler looks like with this change:

image

From Perf trace, you'll notice that mouse down and mouse up events aren't doing much work
Before:
image

After:

image

Cypress tests are passing -

image

Copy link

changeset-bot bot commented Sep 29, 2024

🦋 Changeset detected

Latest commit: b3de0f7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@dnd-kit/core Patch

Not sure what this means? Click here to learn what changesets are.

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

@alloy
Copy link

alloy commented Oct 9, 2024

@dinkinflickaa Can you please run the e2e tests locally and show that they pass? Also, can the e2e tests be amended any way to protect against this regressing in the future? (I assume it would require more tooling, such as integrating React Render Tracker?)

@dinkinflickaa dinkinflickaa force-pushed the dinkinflicka/optimize-rerenders-on-click branch from 0923cc4 to b3de0f7 Compare October 9, 2024 20:17
@dinkinflickaa
Copy link
Author

@alloy Thanks for pointing out tests. This helped me uncover a bug which I have fixed now. Added a screenshot of passing tests to PR description.

@dinkinflickaa dinkinflickaa changed the title Avoid re-rendering nodes on click Avoid re-rendering children on click event Oct 9, 2024
@SudoPlz
Copy link

SudoPlz commented Nov 15, 2024

We're experiencing the same issue in our project, any idea when this will be merged?

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 this pull request may close these issues.

4 participants